Skip to content

feat: enhance the capping of max spans per trace at global level#290

Merged
kotharironak merged 4 commits intomainfrom
global-cap-for-max-span-count
Nov 24, 2021
Merged

feat: enhance the capping of max spans per trace at global level#290
kotharironak merged 4 commits intomainfrom
global-cap-for-max-span-count

Conversation

@kotharironak
Copy link
Copy Markdown
Contributor

@kotharironak kotharironak commented Nov 24, 2021

The rawSpanGrouper, stitch the spans into traces. To cap the number of spans per tace, currently, we have config that controls per-tenant level. e.g

max.span.count = {
  tenant1 = 5
}

As part of this PR, we are enhancing this scheme by adding a global upper limit. As shown in the below example config, tenant1 is applying 5 spans/trace limit, but for other tenants (say tenant2), it will apply 6 spans/trace limit.

default.max.span.count = 6
max.span.count = {
  tenant1 = 5
}

@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 24, 2021

Codecov Report

Merging #290 (56dc177) into main (efa5d17) will increase coverage by 0.15%.
The diff coverage is 86.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #290      +/-   ##
============================================
+ Coverage     79.44%   79.60%   +0.15%     
- Complexity     1265     1268       +3     
============================================
  Files           112      112              
  Lines          4934     4937       +3     
  Branches        452      451       -1     
============================================
+ Hits           3920     3930      +10     
+ Misses          812      806       -6     
+ Partials        202      201       -1     
Flag Coverage Δ
unit 79.60% <86.66%> (+0.15%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../core/rawspansgrouper/RawSpanGrouperConstants.java 0.00% <ø> (ø)
...rtrace/core/rawspansgrouper/RawSpansProcessor.java 81.57% <86.66%> (+6.80%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efa5d17...56dc177. Read the comment docs.

@github-actions

This comment has been minimized.

span.window.store.segment.size.mins = 20
span.window.store.segment.size.mins = ${?SPAN_WINDOW_STORE_SEGMENT_SIZE_MINS}

upper.max.span.count = 6
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we already have max.span.count for per tenant, didn't go for max.span.count.upper.limit as config. Let me know if you have a better name.

span.window.store.segment.size.mins = 20
span.window.store.segment.size.mins = ${?SPAN_WINDOW_STORE_SEGMENT_SIZE_MINS}

upper.max.span.count = 6
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should call this default.max.span.count and the below section can be used as an override per tenant.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense. Made changes.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@kotharironak kotharironak merged commit cdbd7b4 into main Nov 24, 2021
@kotharironak kotharironak deleted the global-cap-for-max-span-count branch November 24, 2021 15:00
@github-actions
Copy link
Copy Markdown

Unit Test Results

  74 files  ±0    74 suites  ±0   1m 2s ⏱️ -2s
392 tests ±0  392 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit cdbd7b4. ± Comparison against base commit efa5d17.

? maxSpanCountMap.get(key.getTenantId())
: Long.MAX_VALUE;

if (inFlightSpansPerTrace >= defaultMaxSpanCountLimit
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought tenant specific config acts as an override the default.
But the condition here doesn't seem to indicate that. We are picking the least of both.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What @ravisingal & I had discussed offline of having a global upper limit, and so I had prior notatin of upper.max.span.count. But, what you are saying is that for a specific tenant, we want a higher limit, that makes sense.

Let me know @avinashkolluru @ravisingal if we want to modify and go with that if that is making more sense.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest made the tenant specific config as an override. It shouldn't matter whether its is lower or higher than the default.

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.

3 participants