feat(node-experimental): Use native OTEL Spans#9161
Conversation
| const sentryRequest = https.request( | ||
| sentryIngestUrl, | ||
| { headers: proxyRequest.headers, method: proxyRequest.method }, | ||
| sentryResponse => { | ||
| sentryResponse.addListener('data', (chunk: Buffer) => { | ||
| proxyResponse.write(chunk, 'binary'); | ||
| sentryResponseChunks.push(chunk); | ||
| }); | ||
|
|
||
| sentryResponse.addListener('end', () => { | ||
| eventCallbackListeners.forEach(listener => { | ||
| const rawSentryResponseBody = Buffer.concat(sentryResponseChunks).toString(); | ||
|
|
||
| const data: SentryRequestCallbackData = { | ||
| envelope: parseEnvelope(proxyRequestBody, new TextEncoder(), new TextDecoder()), | ||
| rawProxyRequestBody: proxyRequestBody, | ||
| rawSentryResponseBody, | ||
| sentryResponseStatusCode: sentryResponse.statusCode, | ||
| }; | ||
|
|
||
| listener(Buffer.from(JSON.stringify(data)).toString('base64')); | ||
| }); | ||
| proxyResponse.end(); | ||
| }); | ||
|
|
||
| sentryResponse.addListener('error', err => { | ||
| throw err; | ||
| }); | ||
|
|
||
| proxyResponse.writeHead(sentryResponse.statusCode || 500, sentryResponse.headers); | ||
| }, | ||
| ); |
Check failure
Code scanning / CodeQL
Server-side request forgery
size-limit report 📦
|
Lms24
left a comment
There was a problem hiding this comment.
Ok, so I took a look at the PR and while I didn't review every single detail (frankly, I think I lack the context for a lot of the more otel-specific APIs), the general concept makes sense to me. Had some questions around the exporter and types but nothing blocking.
Should we apply the scope when a span is started, or when it is finished
I think it's fine (for now) to use the scope from when the span was started. Especially because it's how Otel does it and I think we generally want to stick with Otel in this package.
Another only tangentially related thought while reviewing: We definitely need to add proper docs for the package (however it is called) we release/maintain alongside @sentry/node during v8.
| diag.setLogger(otelLogger, DiagLogLevel.DEBUG); | ||
| } | ||
|
|
||
| if (client) { |
There was a problem hiding this comment.
Just wondering: If client is undefined, should we even do anything in this function?
There was a problem hiding this comment.
good question 😅 I would say right now that is more of a theoretical question, as that is just called by init() right after we called initNode(), so there should always be a client. But once we eventually split this up into more easily consumable parts, we'll need to handle these cases better.
| span.end(); | ||
| } | ||
|
|
||
| _initSpan(span as OtelSpan, spanContext); |
There was a problem hiding this comment.
q: I'm seeing quite a lot of type casting when dealing with otel spans. Is this because Otel types are somehow wrong/too broad?
There was a problem hiding this comment.
this is because @opentelemetry/api and everything related to that passes a basic Span type around, while we often need the spans that are actually generated by @opentelemetry/sdk-trace-base for certain things (because that has some more fields with things we need). But it's something we should look into when we stabilize this/split this up into better reusable parts, ideally we can avoid as much of this as possible 😬
There was a problem hiding this comment.
actually here specifically I'll update it and just pass the regular 'spans' around. that's safer and prob. more correct anyhow!
|
|
||
| function isTransaction(span: Span): span is Transaction { | ||
| return span instanceof Transaction; | ||
| function _initSpan(span: OtelSpan, spanContext: NodeExperimentalSpanContext): void { |
There was a problem hiding this comment.
l: wdyt about calling this something around applySentryAttributesToSpan or similar?
| /** | ||
| * This is a fork of the base Transaction with OTEL specific stuff added. | ||
| */ | ||
| export class NodeExperimentalTransaction extends Transaction { |
There was a problem hiding this comment.
l: iiuc, Transaction::finish shouldn't do anything anymore, right? Should we override it here to print a warning that it noops if it's called? I guess chances are low that users would call finish given that they'll only work with spans, so feel free to disregard.
There was a problem hiding this comment.
actually users should not get a hold of a transaction ever 😅 startTransaction is not exported, and only used in the span exporter.
|
|
||
| this._finishedSpans.push(...spans); | ||
|
|
||
| const remainingSpans = maybeSend(this._finishedSpans); |
There was a problem hiding this comment.
q: When would there be remaining spans and what happens with them? Is it when we have multiple concurrent root spans?
There was a problem hiding this comment.
There are two scenarios, one is "normal" and will happen, and one should not happen but may happen (who knows):
-
When a root span is not finished yet, all the child spans will remain in there. E.g. think one http request in a transaction may be finished and go to the exporter, while the overall transaction is still running. In this case, the http span will remain here until the root span is completed.
-
Somewhere along the way some span was dropped (for whatever reason), so the parent span of this span never gets to the exporter. This should not happen, but 🤷 So in this case we'll eventually clean this up and just discard the span.
There was a problem hiding this comment.
I also added a comment to explain this a bit better!
1531c77 to
5e984c2
Compare
5e984c2 to
613269d
Compare
|
I've updated this based on feedback from @Lms24 , thanks a lot. |
Use regular `Span` type from `@opentelemetry/api` wherever possible.
7c7d4e6 to
385636a
Compare
Lms24
left a comment
There was a problem hiding this comment.
Thanks for applying my feedback and answering my questions!
This PR changes the performance handling of the node-experimental package fundamentally, aligning it even more with the OpenTelemetry model.
The core changes are:
startSpan/startInactiveSpancreate Otel spans now, not Sentry Spans/TransactionsHow does transaction/span creation work now
In the old model, we would start transactions/spans in the span processor
onStartandonEndhook. This required us to keep track of the parent Sentry span, as we need it to callparentSpan.startSpan()inonStart. Since it can be tricky to know when a span is not needed anymore as a parent etc, this made garbage collection harder and messier, and also required us to still sprinkle Sentry spans/transactions everywhere through our code.In the new model, only minimal processing is done in the span processor, and importantly, we do not create any Sentry spans yet. We store some additional data we need later in a WeakMap associated to the (Otel) span.
Then we leverage the underlying
BatchSpanProcessorfrom OTEL, which collects spans together and sends them for processing to aSpanExporter. So only finished spans end up in our span processor. Our custom span exporter does the following:For now I copied most of the stuff from opentelemetry-node over, eventually we can merge most of this together probably and export the parts from opentelemetry-node.
How do breadcrumbs work
We now pick all events added to spans and add them as breadcrumbs.
For this, we walk up the tree of spans up to the root and collect all breadcrumbs together. We use a special JSON field for now to actually store the breadcrumbs data (TODO: Maybe there is a better way to do this...).
When we add a breadcrumb, we actually always add it to the root span for now, not the active span. The reason is that this works better with our mental model of breadcrumbs, where anything that happens in this root span is relevant. But we'll also pick up any other events added by otel instrumentation along the way.
Open questions
onEnd. But not 100% sure how this would work with parallel spans, needs to be tested I guess.