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

feat(otel): Add basic SentrySpanProcessor #6023

Merged
merged 4 commits into from Oct 28, 2022
Merged

Conversation

AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented Oct 24, 2022

Implement a basic SpanProcessor for OpenTelemetry.

This is very much WIP! The usage will be something like this:

import { SentrySpanProcessor} from '@sentry/opentelemetry-node';

otelProvider.addSpanProcessor(new SentrySpanProcessor());

The processor will automatically convert all otel spans into sentry transactions/spans, and keep the correct parent-child relationship.

ref #6000

@github-actions
Copy link
Contributor

github-actions bot commented Oct 24, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.49 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 60.35 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.12 KB (-0.02% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 53.68 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 19.86 KB (0%)
@sentry/browser - Webpack (minified) 65.09 KB (0%)
@sentry/react - Webpack (gzipped + minified) 19.89 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 45.78 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.24 KB (-0.01% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.62 KB (-0.01% 🔽)

@mydea mydea force-pushed the abhi-basic-span-processor branch 2 times, most recently from 5ed079c to e53ee52 Compare October 27, 2022 11:44
@mydea
Copy link
Member

mydea commented Oct 27, 2022

Pushed changes:

  1. SpanProcessor now does not touch Sentry scope's active span
  2. Update tests to work (do not use global otel stuff, use NodeTracerProvider for tests)

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;
Copy link
Member

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?

Copy link
Member Author

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>();
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

@mydea mydea left a 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!

@AbhiPrasad AbhiPrasad marked this pull request as ready for review October 28, 2022 08:56
@AbhiPrasad AbhiPrasad enabled auto-merge (squash) October 28, 2022 09:09
@AbhiPrasad AbhiPrasad merged commit 8d7e050 into master Oct 28, 2022
@AbhiPrasad AbhiPrasad deleted the abhi-basic-span-processor branch October 28, 2022 09:12
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.

None yet

2 participants