-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(otel): Add basic SentrySpanProcessor #6023
Conversation
c9bd9dc
to
fec3f55
Compare
size-limit report 📦
|
5ed079c
to
e53ee52
Compare
Pushed changes:
Especially 1. allowed to remove some stuff (no need to keep the parent span in the internal map anymore, for example). |
startChild( | ||
spanContext?: Pick<SpanContext, Exclude<keyof SpanContext, 'spanId' | 'sampled' | 'traceId' | 'parentSpanId'>>, | ||
): Span; | ||
startChild(spanContext?: Pick<SpanContext, Exclude<keyof SpanContext, 'sampled' | 'traceId' | 'parentSpanId'>>): Span; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was missed in #6028?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes 🤦
*/ | ||
export class SentrySpanProcessor implements OtelSpanProcessor { | ||
// public only for testing | ||
public readonly _map: Map<SentrySpan['spanId'], SentrySpan> = new Map<SentrySpan['spanId'], SentrySpan>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not super nice to make this public just so we can use it in tests, so if there are other ideas for this, let me know. It works, at least 😅
Also FYI I changed this to a Map
instead of {}
, as that makes delete()
nicer imho. I think there shouldn't really be a perf difference, but let me know if we should rather leave this a POJO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a map seems fine to me - cleaner pattern overall.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I think this is a solid start!
e53ee52
to
d558320
Compare
Implement a basic
SpanProcessor
for OpenTelemetry.This is very much WIP! The usage will be something like this:
The processor will automatically convert all otel spans into sentry transactions/spans, and keep the correct parent-child relationship.
ref #6000