Skip to content

feat: Add initial OpenTelemetry support#537

Merged
tonyo merged 92 commits intomasterfrom
otel
Jan 31, 2023
Merged

feat: Add initial OpenTelemetry support#537
tonyo merged 92 commits intomasterfrom
otel

Conversation

@cleptric
Copy link
Copy Markdown
Member

@cleptric cleptric commented Jan 19, 2023

This PR adds two main integration components (along with all the main supporting functions to enrich Sentry spans with OTel data):

Main discussion: getsentry/sentry#40712

Closes #526, closes #486.

A list of open issues with the implementation:

  1. Dynamic Sampling Context propagation from nested spans doesn't work properly. Main reason: no publicly exposed mechanism to get the transaction (as in, a root span) for the given nested span.
    [OTel] Dynamic Sampling Context propagation does not work for nested spans #553
  2. Baggage parsing bug is not yet fixed or acknowledged: Error while parsing Baggage header with percent-encoded whitespace open-telemetry/opentelemetry-go#3601 We might want to use our vendored implementation and fix the bug there.
    [OTel] Baggage parsed incorrectly if it contains encoded whitespaces ("%20") #554
  3. Associating errors with transactions is generally implemented (see otel/event_processor.go), but there's a caveat: the EventHint of the event processor has to contain a valid Context with a set OpenTelemetry span. This means that e.g. messages captured with sentry.CaptureMessage or hub.CaptureMessage won't be linked to Sentry spans at the moment.
    [OTel] Expose context-aware methods for capturing events in public API #555
  4. "instrumenter" configuration in ClientOptions -- we need to confirm whether we need that.
    [OTel] Add "instrumenter" configuration option #556
  5. Span attributes/description for special cases (HTTP, DB, RPC, etc.) might be incomplete.
    [OTel] Improve HTTP Span attributes in SpanProcessor #557

Comment thread otel/internal/utils/sentryrequest.go
Comment thread otel/propagator.go Outdated
//
// https://opentelemetry.io/docs/reference/specification/context/api-propagators/#inject
func (p sentryPropagator) Inject(ctx context.Context, carrier propagation.TextMapCarrier) {
sentry.Logger.Printf("\n--- Propagator Inject\nContext: %#v\nCarrier: %#v\n", ctx, carrier)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This might be super noisy for users. I use Debug: true a lot, so maybe we could reduce the output a little bit.

@tonyo tonyo marked this pull request as ready for review January 31, 2023 10:43
Copy link
Copy Markdown
Contributor

@tonyo tonyo left a comment

Choose a reason for hiding this comment

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

🚢 🚂 🚀

@tonyo tonyo merged commit d362c36 into master Jan 31, 2023
@tonyo tonyo deleted the otel branch January 31, 2023 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Topic: OpenTelemetry Issue/PR related to OpenTelemetry integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[OpenTelemetry] Implement OTel Propagator [GO] Add basic OpenTelemetry support

2 participants