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

[processor/tailsamplingprocessor] Add a max_spans configuration for the span_count policy of tailsamplingprocessor #18483

Conversation

mizhexiaoxiao
Copy link
Contributor

Description:
Add a max_spans configuration for the span_count policy of tailsamplingprocessor

Link to tracking Issue: #18215

Testing:

Documentation:

@mizhexiaoxiao mizhexiaoxiao requested a review from a team February 9, 2023 10:09
@github-actions github-actions bot added the processor/tailsampling Tail sampling processor label Feb 9, 2023
@runforesight
Copy link

runforesight bot commented Feb 9, 2023

Foresight Summary

    
Major Impacts

build-and-test-windows duration(5 seconds) has decreased 43 minutes 15 seconds compared to main branch avg(43 minutes 20 seconds).
View More Details

⭕  build-and-test-windows workflow has finished in 5 seconds (43 minutes 15 seconds less than main branch avg.) and finished at 2nd Mar, 2023.


Job Failed Steps Tests
windows-unittest-matrix -     🔗  N/A See Details
windows-unittest -     🔗  N/A See Details

✅  telemetrygen workflow has finished in 1 minute 13 seconds (1 minute 54 seconds less than main branch avg.) and finished at 2nd Mar, 2023.


Job Failed Steps Tests
publish-latest -     🔗  N/A See Details
build-dev -     🔗  N/A See Details
publish-stable -     🔗  N/A See Details

✅  check-links workflow has finished in 1 minute 43 seconds (48 seconds less than main branch avg.) and finished at 2nd Mar, 2023.


Job Failed Steps Tests
changed files -     🔗  N/A See Details
check-links -     🔗  N/A See Details

✅  changelog workflow has finished in 2 minutes 2 seconds (44 seconds less than main branch avg.) and finished at 2nd Mar, 2023.


Job Failed Steps Tests
changelog -     🔗  N/A See Details

✅  build-and-test workflow has finished in 33 minutes 10 seconds (38 minutes 27 seconds less than main branch avg.) and finished at 2nd Mar, 2023.


Job Failed Steps Tests
unittest-matrix (1.19, internal) -     🔗  ✅ 581  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, internal) -     🔗  ✅ 581  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, processor) -     🔗  ✅ 1543  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, processor) -     🔗  ✅ 1543  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, extension) -     🔗  ✅ 537  ❌ 0  ⏭ 0    🔗 See Details
correctness-metrics -     🔗  ✅ 2  ❌ 0  ⏭ 0    🔗 See Details
correctness-traces -     🔗  ✅ 17  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, extension) -     🔗  ✅ 537  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, receiver-0) -     🔗  ✅ 2580  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, receiver-0) -     🔗  ✅ 2580  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, exporter) -     🔗  ✅ 2466  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, exporter) -     🔗  ✅ 2466  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, receiver-1) -     🔗  ✅ 1937  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, receiver-1) -     🔗  ✅ 1937  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, other) -     🔗  ✅ 4789  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, other) -     🔗  ✅ 4789  ❌ 0  ⏭ 0    🔗 See Details
integration-tests -     🔗  ✅ 55  ❌ 0  ⏭ 0    🔗 See Details
setup-environment -     🔗  N/A See Details
check-collector-module-version -     🔗  N/A See Details
check-codeowners -     🔗  N/A See Details
checks -     🔗  N/A See Details
lint-matrix (receiver-0) -     🔗  N/A See Details
lint-matrix (receiver-1) -     🔗  N/A See Details
lint-matrix (processor) -     🔗  N/A See Details
lint-matrix (exporter) -     🔗  N/A See Details
lint-matrix (extension) -     🔗  N/A See Details
lint-matrix (internal) -     🔗  N/A See Details
lint-matrix (other) -     🔗  N/A See Details
build-examples -     🔗  N/A See Details
lint -     🔗  N/A See Details
unittest (1.20) -     🔗  N/A See Details
unittest (1.19) -     🔗  N/A See Details
cross-compile (darwin, amd64) -     🔗  N/A See Details
cross-compile (darwin, arm64) -     🔗  N/A See Details
cross-compile (linux, 386) -     🔗  N/A See Details
cross-compile (linux, amd64) -     🔗  N/A See Details
cross-compile (linux, arm) -     🔗  N/A See Details
cross-compile (linux, arm64) -     🔗  N/A See Details
cross-compile (linux, ppc64le) -     🔗  N/A See Details
cross-compile (windows, 386) -     🔗  N/A See Details
cross-compile (windows, amd64) -     🔗  N/A See Details
build-package (deb) -     🔗  N/A See Details
build-package (rpm) -     🔗  N/A See Details
windows-msi -     🔗  N/A See Details
publish-check -     🔗  N/A See Details
publish-stable -     🔗  N/A See Details
publish-dev -     🔗  N/A See Details

✅  prometheus-compliance-tests workflow has finished in 3 minutes 16 seconds (5 minutes 46 seconds less than main branch avg.) and finished at 2nd Mar, 2023.


Job Failed Steps Tests
prometheus-compliance-tests -     🔗  ✅ 21  ❌ 0  ⏭ 0    🔗 See Details

✅  load-tests workflow has finished in 7 minutes 23 seconds (9 minutes 58 seconds less than main branch avg.) and finished at 2nd Mar, 2023.


Job Failed Steps Tests
loadtest (TestTraceAttributesProcessor) -     🔗  ✅ 3  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestIdleMode) -     🔗  ✅ 1  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestMetric10kDPS|TestMetricsFromFile) -     🔗  ✅ 6  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestTraceNoBackend10kSPS|TestTrace1kSPSWithAttrs) -     🔗  ✅ 8  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestTraceBallast1kSPSWithAttrs|TestTraceBallast1kSPSAddAttrs) -     🔗  ✅ 10  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestMetricResourceProcessor|TestTrace10kSPS) -     🔗  ✅ 12  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestBallastMemory|TestLog10kDPS) -     🔗  ✅ 18  ❌ 0  ⏭ 0    🔗 See Details
setup-environment -     🔗  N/A See Details

✅  e2e-tests workflow has finished in 12 minutes 51 seconds (3 minutes 41 seconds less than main branch avg.) and finished at 2nd Mar, 2023.


Job Failed Steps Tests
kubernetes-test (v1.26.0) -     🔗  N/A See Details
kubernetes-test (v1.25.3) -     🔗  N/A See Details
kubernetes-test (v1.24.7) -     🔗  N/A See Details
kubernetes-test (v1.23.13) -     🔗  N/A See Details

🔎 See details on Foresight

*You can configure Foresight comments in your organization settings page.

if int(traceData.SpanCount.Load()) >= int(c.minSpans) {
spanCount := traceData.SpanCount.Load()
switch {
case c.maxSpans == 0 && int(spanCount) >= int(c.minSpans):
Copy link
Member

Choose a reason for hiding this comment

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

You are potentially casting the spanCount to int multiple times, and there's never a situation you are not casting it. Why not just cast it on line 43 directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it would be better

@@ -22,7 +22,7 @@ Multiple policies exist today and it is straight forward to add more. These incl
- `string_attribute`: Sample based on string attributes (resource and record) value matches, both exact and regex value matches are supported
- `trace_state`: Sample based on [TraceState](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#tracestate) value matches
- `rate_limiting`: Sample based on rate
- `span_count`: Sample based on the minimum number of spans within a batch. If all traces within the batch have less number of spans than the threshold, the batch will not be sampled.
- `span_count`: Sample based on the minimum and maximum number of spans. If the sum of all spans in the trace is outside the range threshold, the trace will not be sampled.
Copy link
Member

Choose a reason for hiding this comment

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

I think it should mention that the boundaries are inclusive. Like, max_spans=20 will return spans with 20, but not 21.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UPDATE mentions inclusion of minimum and maximum values

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

LGTM

@yehaotian
Copy link

Thanks for the work, any timeline this can be released?

@jiaojialin
Copy link

Hello , @jpkrohling could you please take another look? this is a very useful feature that could make a difference immediately. Would love to have it ASAP. thanks

@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2023

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@jiaojialin
Copy link

Hello, this PR is pretty simple and straightforward, could someone please take another look and approve it? Thanks cc @jpkrohling

@TylerHelmuth TylerHelmuth removed the Stale label Mar 1, 2023
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

@mizhexiaoxiao please take a look at the merge conflicts

@jpkrohling
Copy link
Member

Sorry for the delay in reviewing this. LGTM, this will be merged once the merge conflicts are solved and CI is green.

@jiaojialin
Copy link

Thanks so much @mizhexiaoxiao @TylerHelmuth @jpkrohling ! Looking forward to this feature the be released! @mizhexiaoxiao could you please resolve the conflicts at your earliest convenience and merge it? thanks so much!!

@mizhexiaoxiao
Copy link
Contributor Author

Ready to merge!

@TylerHelmuth TylerHelmuth merged commit 2dd7f69 into open-telemetry:main Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
processor/tailsampling Tail sampling processor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants