-
Notifications
You must be signed in to change notification settings - Fork 409
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: Added ability to propagate traceparent and tracestate on incoming server/consumer spans and outgoing client http and producer spans #2958
Conversation
@@ -238,7 +238,6 @@ function createConsumerWrapper({ shim, spec, consumer }) { | |||
* finalizes transaction name and ends transaction | |||
*/ | |||
function endTransaction() { | |||
tx.finalizeName(null) // Use existing partial name. |
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 can remove this but realized this was redundant as transaction.end does this as well
@@ -24,24 +25,20 @@ module.exports = function setupOtel(agent, logger = defaultLogger) { | |||
|
|||
createOtelLogger(logger, agent.config) | |||
|
|||
const provider = new BasicTracerProvider({ | |||
opentelemetry.trace.setGlobalTracerProvider(new BasicTracerProvider({ |
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.
this was done in prep for the removal of the register method, see
…ng server/consumer spans and outgoing client http and producer spans.
587e1cc
to
8190564
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2958 +/- ##
==========================================
- Coverage 79.87% 79.85% -0.03%
==========================================
Files 307 308 +1
Lines 47588 47650 +62
==========================================
+ Hits 38012 38050 +38
- Misses 9576 9600 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 don't see anything blocking, but I have a couple of questions.
@@ -25,7 +26,8 @@ const { | |||
|
|||
function createConsumerSegment(agent, otelSpan) { | |||
const attrs = otelSpan.attributes | |||
const transaction = new Transaction(agent) | |||
const spanContext = otelSpan.spanContext() | |||
const transaction = new Transaction(agent, spanContext?.traceId) |
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.
This is a question I had when I did the initial otel research: does our infrastructure support otel's trace id format? I'm pretty sure otel's is a plain uuuidv4 sans dashes. Ours isn't:
node-newrelic/lib/util/hashes.js
Lines 87 to 115 in 8943672
function makeId(length = 16) { | |
// length is number of hex characters, which multiplied by 4 is the number of | |
// bits, then divided by 8 is number of bytes. Or just divide by 2 | |
const numBytes = Math.ceil(length / 2) | |
const randBytes = new Uint8Array(numBytes) | |
// Generate random bytes one 32-bit integer at a time | |
const numInts = Math.ceil(numBytes / 4) // 32 bit integers are 4 bytes | |
for (let i = 0; i < numInts; i++) { | |
const int = randInt32() | |
const bytes = int32ToByteArray(int) | |
for (let j = 0; j < 4; j++) { | |
// This could "overflow" since we're iterating over the number of ints, which could | |
// be more data than needed. But out-of-bound index assignment on typed arrays are | |
// discarded | |
randBytes[i * 4 + j] = bytes[j] | |
} | |
} | |
// Convert the byte array to a hex string | |
let id = '' | |
for (let i = 0; i < randBytes.length; i++) { | |
id += byteToHex[randBytes[i]] | |
} | |
// For odd number lengths, we may get an extra character since byteToHex returns two | |
// characters, so trim to the desired length. | |
return id.substring(0, length) | |
} |
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 doesn't have an issue. I've tested this branch on apps and its linking 2 entities and the trace id from otel is on all spans
Description
This PR finalizes the propagation of traceparent/tracestate headers. For incoming server spans it's a 2 step process, instrumentation will call
propagation.extract
to get the new context based on traceparent/tracestate. Then when a server span's transaction is started it will accept the payload and assign the necessary attributes on the transaction. For this to be supported I had to enhanceTransaction
to pass in the traceId if present. This is so the trace id created from otel instrumentation will also be the transaction trace id. Previously, this was a lazy property and created a unique id the first time it was accessed.For outging client spans, it calls
propagation.inject
which will get the relevant traceId, spanId, sampled flag on the active span and inject into the outgoing headers. I also had to enhanceTraceSegment
to pass in id(spanId) if present.How to Test
unit and versioned tests assert the new logic. I also manually tested with our new examples and have confirmed DT linking it done accordingly.
Related Issues
Closes #2662