-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add limit for internal span capture #974
Add limit for internal span capture #974
Conversation
@@ -140,6 +140,7 @@ internal class SpanServiceImpl( | |||
} | |||
|
|||
companion object { | |||
const val MAX_INTERNAL_SPANS_PER_SESSION = 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should bump this limit - network requests will easily push past this, and we allow 1000 per session per domain, IIRC. I'd go 3000 to start off with, and maybe look at existing data to inform this limit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I bumped this to 5000 & we can adjust from there. Really it's just a backstop to avoid stupid amounts of spans getting logged & in practice I don't think this will be reached for the vast majority of payloads
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. We may want to bump the limit though because network requests will blow past this easily
405f79d
to
2fecce9
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## integration/v2-migration #974 +/- ##
=========================================================
Coverage 79.78% 79.78%
=========================================================
Files 406 406
Lines 10798 10804 +6
Branches 1715 1716 +1
=========================================================
+ Hits 8615 8620 +5
Misses 1406 1406
- Partials 777 778 +1
|
Goal
Adds a limit of 1000 for the number of internal spans that we capture. I'm open to discussion on the exact limit we enforce, but noticed during my validation of this migration that the number of internal spans was unbounded.
Technically we should always bound the number of spans in the
DataSource
implementation, but in practice I'd like to add this as a backstop to prevent the payload from getting overloaded.Testing
Added unit tests.