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

Span Processors do not respect sampled flag when exporting #2394

Closed
quickgiant opened this issue Aug 3, 2021 · 4 comments · Fixed by #2396
Closed

Span Processors do not respect sampled flag when exporting #2394

quickgiant opened this issue Aug 3, 2021 · 4 comments · Fixed by #2396
Labels
bug Something isn't working

Comments

@quickgiant
Copy link
Contributor

quickgiant commented Aug 3, 2021

What version of OpenTelemetry are you using?

@opentelemetry/[email protected]
@opentelemetry/[email protected]
@opentelemetry/[email protected]

What version of Node are you using?

14.17.3

What did you do?

  • Set up a BasicTracerProvider
  • Add a sampler that downsamples traces using the RECORD_ONLY sampling decision
  • Add a BatchSpanProcessor or SimpleSpanProcessor
  • Add an exporter, either a dummy that logs spans or an actual exporter

What did you expect to see?

Expected that only sampled spans would be exported, per the spec, and as described by doc comments on SimpleSpanProcessor and the SpanExporter interface.

What did you see instead?

All spans are exported.

Additional context

It seems that the sampled flag was originally being respected by Span Processors, but the logic was removed in this PR: #798. The rationale at the time was that the check was redundant, since Tracer was also checking the sampled flag and preventing unsampled spans from being recorded.

In a later PR #1058, the Tracer check was changed to differentiate between not recorded and not sampled, which resulted in no check being made for the sampled flag.

Looks like the original check in Span Processors just needs to be re-added, which I would be willing to take on and make a PR for. There should probably also be a test for this to ensure that the spec is met.

@Flarna
Copy link
Member

Flarna commented Aug 4, 2021

The sampling decision is checked in Tracer and it creates a NonRecordingSpan (via ) or a "real" Span depending on this.
SpanProcessor.onStart() is called in Span constructor.

According to spec that seems to be ok. Spec does not require that recorded spans are not exported (should not instead

Which sampler do you use and which return value is used by your sampler? Is it NOT_RECORD or RECORD?

@quickgiant
Copy link
Contributor Author

In the "Sampling" section of the specification, the flag you're referring to is called IsRecording, but there's a second flag, Sampled, which is the one I'm referring to. The sampling decision has 3 possible values, DROP, RECORD_ONLY, and RECORD_AND_SAMPLE. The check in Tracer handles the DROP decision, but cannot differentiate between RECORD_ONLY and RECORD_AND_SAMPLE because that handling is the Span Processor's responsibility. The table in the "Sampling" section makes the point clearer, where if IsRecording is true but Sampled is false, the span should reach the Span Processor but not the Span Exporter. It's also stated in the text: "Span Processor MUST receive only those spans which have this field [IsRecording] set to true. However, Span Exporter SHOULD NOT receive them unless the Sampled flag was also set."

@Flarna
Copy link
Member

Flarna commented Aug 4, 2021

@quickgiant Yes I know the three states. But looking into all samples in this repo they either return RECORD_AND_SAMPLED or NOT_RECORD. So the case fixed in #2396 will not change anything if samplers from this repo are used.

@quickgiant
Copy link
Contributor Author

Yes, the bug was encountered when using a custom sampler that uses RECORD_ONLY. I see how the included samplers would hide the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants