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

[otel] integration should sync sampling decision to/from otel in the "parentless" case #678

Closed
costela opened this issue Jul 28, 2023 · 2 comments · May be fixed by #679
Closed

[otel] integration should sync sampling decision to/from otel in the "parentless" case #678

costela opened this issue Jul 28, 2023 · 2 comments · May be fixed by #679

Comments

@costela
Copy link

costela commented Jul 28, 2023

Summary

Currently, using something like the following seems to lead to unexpected results:

	trace.NewTracerProvider(
		trace.WithSampler(
			trace.ParentBased(
				trace.TraceIDRatioBased(cfg.SampleRate),
			),
		),
		trace.WithSpanProcessor(sentryotel.NewSentrySpanProcessor()),
	)

The decision from the otel sampler is ignored by sentry, which then samples again based on its own logic, leaving the otel and sentry samplig decisions disconnected.

Using only sentry.ClientOptions.TracesSampler is not an option for similar reasons: its decision does not get passed back to otel, so otel's Span.IsSampled() returns an unrelated value to sentry's decision (so that you end up - e.g. - using a traceID in exemplars and logs that never actually got sent to sentry, or the other way around).

The otel and sentry sides seem to agree in the case where a parent span could be found.

Am I holding it wrong? 😅

Steps To Reproduce

Initialize otel with:

trace.NewTracerProvider(
	trace.WithSampler(trace.AlwaysSample()),
	trace.WithSpanProcessor(sentryotel.NewSentrySpanProcessor()),
)

Initialize sentry with:

sentry.ClientOptions{
	//...
	EnableTracing:    true,
	TracesSampleRate: 0.0,
	//...
}

All traces created with otel.Tracer("").Start() will have span.IsSampled() == true, but none of them will be sent to sentry.

Switching the to NeverSample()/TracesSampleRate: 1.0 causes the opposite problem.

Expected Behavior

Sentry should respect OTel's span.IsSampled() and only sample trances if and only if it is true.

(The other way around seems a bit more difficult to pull off?)

Environment

SDK

  • sentry-go 0.22.0:
  • Go version: 1.20
  • Using Go Modules? yes

Sentry

  • Using hosted Sentry in sentry.io? yes

Additional context

@costela
Copy link
Author

costela commented Aug 23, 2023

Added a bit more context here: #679 (comment)

The current state in that PR would imply the docs have to be updated:

  • using otel+sentryhttp needs further options to sentryhttp
  • respecting otel sampling requires TracesSampleRate to always be 0 (otherwise both sampling mechanisms may diverge)

@cleptric
Copy link
Member

cleptric commented Oct 4, 2023

Closing as #679 is moving in another direction.
TL;DR is that we won't support mixing Sentry instrumentation with Otel instrumentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants