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 option to export unsampled spans from span processors #6057

Merged
merged 10 commits into from
Jan 4, 2024

Conversation

HaloFour
Copy link
Contributor

@HaloFour HaloFour commented Dec 7, 2023

Adds an option to the SimpleSpanProcessor and BatchSpanProcessor which allows unsampled spans to be exported.

SpanExporter spanExporter = ...;

SimpleSpanProcessor simpleSpanProcessor = SimpleSpanProcessor.builder(spanExporter)
        .setExportUnsampledSpans(true)
        .build();

BatchSpanProcessor batchSpanProcessor = BatchSpanProcessor.builder(spanExporter)
        .setExportUnsampledSpans(true)
        .build();

Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (b4ed532) 90.99% compared to head (80284b4) 90.98%.
Report is 4 commits behind head on main.

Files Patch % Lines
...elemetry/sdk/trace/export/SimpleSpanProcessor.java 83.33% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6057      +/-   ##
============================================
- Coverage     90.99%   90.98%   -0.02%     
- Complexity     5687     5701      +14     
============================================
  Files           628      630       +2     
  Lines         16675    16708      +33     
  Branches       1654     1656       +2     
============================================
+ Hits          15174    15202      +28     
- Misses         1047     1049       +2     
- Partials        454      457       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

I support some version of this change. We talked in the TC about whether the spec needs to explicitly allow all SDK configuration options with normative "MAY" language, and agreed that the answer is no. Maintainers should be careful about adding options that obviously conflict with the spec, and about adding surface area which is hard to maintain, but there is plenty of precedent for language implementations adding options which are not explicitly defined in the spec.

In this case, I've heard this type of issue discussed a number of times. I think its easy to accommodate and maintain, and conceptually makes sense.

* Sets whether unsampled spans should be exported. If unset, unsampled spans will not be
* exported.
*/
public BatchSpanProcessorBuilder exportUnsampledSpans(boolean exportUnsampledSpans) {
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about this.. I think it would be more flexible / future proof to allow the user to set an optional predicate for deciding whether or not a span is exported. Signature might be one of the following:

// allow user to make decision using full span
public BatchSpanProcessorBuilder setExportPredicate(Predicate<ReadableSpan>)
// allow user to make decision using full span context
public BatchSpanProcessorBuilder setExportPredicate(Predicate<SpanContext>)

Was originally thinking the predicate should accept SamplingDecision, but SamplingDecision ins't available in BatchSpanProcessor#onEnd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hah, I started with a Predicate<ReadableSpan> and backed away from that thinking it would be considered too complicated. I'd be happy with that approach.

I was also thinking we'd want the processor to always drop unrecorded spans? That'd be indicated by an invalid span context, right?

Copy link
Member

Choose a reason for hiding this comment

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

I was also thinking we'd want the processor to always drop unrecorded spans? That'd be indicated by an invalid span context, right?

Yeah, the span never makes it the span processor if its not recorded (see SdkSpanBuilder) so no way for the span processor to influence this.

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

I support 👍

@HaloFour HaloFour marked this pull request as ready for review December 7, 2023 23:23
@HaloFour HaloFour requested a review from a team December 7, 2023 23:23
@HaloFour
Copy link
Contributor Author

HaloFour commented Dec 7, 2023

I slightly prefer the name setExportFilter to setExportPredicate, fits more with Java streams API.

@jack-berg
Copy link
Member

@open-telemetry/java-approvers can you take a look at this? Would be good to get this in before the 1.34.0 release.

@jack-berg jack-berg added this to the 1.34.0 milestone Jan 3, 2024
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

any thoughts on how to this would/could work with autoconfigure?

@jack-berg
Copy link
Member

any thoughts on how to this would/could work with autoconfigure?

There's obviously no spec'd env var for this type of thing. We could add a java specific env variable, but I'm reluctant to do that because this isn't a java specific problem.

Could use the new addSpanProcessorCustomizer, but would need to add toBuilder() methods to allow the user to re-configure the autoconfigured batch processor.

@trask
Copy link
Member

trask commented Jan 3, 2024

Could use the new addSpanProcessorCustomizer, but would need to add toBuilder() methods to allow the user to re-configure the autoconfigured batch processor.

sounds like a good path forward if/when we want 👍

@trask
Copy link
Member

trask commented Jan 3, 2024

the generalization of this PR to Predicate<ReadableSpan> feels duplicative of addSpanProcessorCustomizer (except for the part about unsampled spans). do you think the generalization is worth it, or should we limit the BatchSpanProcessor flag to just toggling the sampling behavior?

@jack-berg
Copy link
Member

the generalization of this PR to Predicate feels duplicative of addSpanProcessorCustomizer (except for the part about unsampled spans).

You're saying that you can accomplish the same thing by using addSpanProcessorCustomizer to wrap the batch span processor with a span processor which applies the predicate? I don't think this is possible because the goal of this PR is to modify batch span processor to cast a wider net of spans to export that the current predicate of span == null || !span.getSpanContext().isSampled(). A wrapping span processor would only be able to narrow the predicate further.

@trask
Copy link
Member

trask commented Jan 3, 2024

ya, sorry, I was asking what you thought about limiting this PR to just a configurable (boolean) flag on BatchSpanProcessor that determines whether it drops unsampled spans. if you want a more complex filter you can use addSpanProcessorCustomizer (in addition to the new flag on BatchSpanProcessor)

@jack-berg
Copy link
Member

I get what you're saying now: if we only exposed a boolean to indicate that batch span processor should export unsampled spans, then you can effectively accomplish the more general Predicate<ReadableSpan> case by setting batch span processor to export unsampled spans, and wrapping it with another processor which applies your custom predicate.

Its a good point and I agree. @HaloFour can you revert to the original boolean option you started with before I derailed you in this convo? If not, I'm happy to it.

@HaloFour
Copy link
Contributor Author

HaloFour commented Jan 3, 2024

@jack-berg

Shame, I kinda like the power afforded by the general filter.

How does addSpanProcessorCustomizer work? The only code I've found seems to be a part of the autoconfiguration customizer. What if you aren't using that to configure the SDK?

@HaloFour
Copy link
Contributor Author

HaloFour commented Jan 4, 2024

@jack-berg

Nevermind, I get it. With the BatchSpanProcessor no longer filtering out unsampled spans it would be trivial to put a delegating SpanProcessor in front of it that would be responsible for arbitrary filtering.

I am curious as to how one would be expected to do this with the autoconfigure API, though. Unless a generic toBuilder() method is added you'd have to first negotiate the type of the processor and then call whatever API is provided to amend/mutate it to one that doesn't filter unsampled spans. Or would the SDK provide an environment variable to support toggling the unsampled span filtering, knowing that this isn't really advertised functionality per the spec?

@HaloFour
Copy link
Contributor Author

HaloFour commented Jan 4, 2024

ack, rebase fail

@HaloFour HaloFour force-pushed the allow-export-unsampled-spans branch from 25c30f8 to 89a0f04 Compare January 4, 2024 14:34
@jack-berg
Copy link
Member

I am curious as to how one would be expected to do this with the autoconfigure API, though. Unless a generic toBuilder() method is added you'd have to first negotiate the type of the processor and then call whatever API is provided to amend/mutate it to one that doesn't filter unsampled spans. Or would the SDK provide an environment variable to support toggling the unsampled span filtering, knowing that this isn't really advertised functionality per the spec?

Yup I came up with the same set of options 🙂 (see comment). I prefer adding a toBuilder() method because:

There's obviously no spec'd env var for this type of thing. We could add a java specific env variable, but I'm reluctant to do that because this isn't a java specific problem.

@HaloFour
Copy link
Contributor Author

HaloFour commented Jan 4, 2024

@jack-berg

Want me to add toBuilder here, or in a follow-up PR?

@jack-berg
Copy link
Member

I'd say in a followup, unless you know you need to use autoconfigure for your use case.

@jack-berg jack-berg merged commit 07351a2 into open-telemetry:main Jan 4, 2024
17 of 18 checks passed
@HaloFour HaloFour deleted the allow-export-unsampled-spans branch January 4, 2024 21:23
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