Skip to content
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

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

fractalwrench
Copy link
Contributor

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.

@fractalwrench fractalwrench requested a review from a team as a code owner June 19, 2024 13:14
@@ -140,6 +140,7 @@ internal class SpanServiceImpl(
}

companion object {
const val MAX_INTERNAL_SPANS_PER_SESSION = 1000
Copy link
Collaborator

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?

Copy link
Contributor Author

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

Copy link
Collaborator

@bidetofevil bidetofevil left a 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

Copy link

codecov bot commented Jun 20, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.78%. Comparing base (b405fcd) to head (2fecce9).

Additional details and impacted files

Impacted file tree graph

@@                    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     
Files Coverage Δ
...droid/embracesdk/internal/spans/SpanServiceImpl.kt 85.45% <ø> (ø)
...mbracesdk/internal/spans/CurrentSessionSpanImpl.kt 84.21% <85.71%> (+0.87%) ⬆️

... and 2 files with indirect coverage changes

@fractalwrench fractalwrench merged commit 35a1238 into integration/v2-migration Jun 20, 2024
3 checks passed
@fractalwrench fractalwrench deleted the add-internal-span-limit branch June 20, 2024 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants