NH-69217 sw.transaction attribute on otlp traces, metrics #307
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
tammy-baylis-swi
force-pushed
the
NH-69217-sw-trans-attr
branch
from
d184aa4 to
4e78a0b
Compare
tammy-baylis-swi
force-pushed
the
NH-69217-sw-trans-attr
branch
from
4e78a0b to
50323d9
Compare
tammy-baylis-swi
force-pushed
the
NH-69217-sw-trans-attr
branch
from
990743e to
ed02457
Compare
tammy-baylis-swi
changed the title
| @@ -415,6 +419,35 @@ def add_tracestate_capture_to_attributes_dict( | |||
| ) | |||
| return attributes_dict | |||
|
|
|||
| def calculate_otlp_transaction_name( | |||
There was a problem hiding this comment.
This new sampler function duplicates what's in the otlp metrics processor. If this should be split it out into a shared place, I'm not sure where to put it. Maybe a top-level util like W3CTransformer?
There was a problem hiding this comment.
Hmm yes good point, it would be nice to have it shared, although W3C... is very much an HTTP header concern while transaction naming is our SWO-specific concept, so I wouldn't put it there.
There is the top-level apm_txnname_manager, and some other transaction name classes under trace/ that might fit the bill. But you may want to just defer the refactoring until you've had a chance to investigate the idea of subclassing the OTel exporter in order to support custom SDK naming for lambda (which can definitely be a future iteration).
There was a problem hiding this comment.
Thanks! I couldn't remember that exact piece. Made a ticket for subclassing: https://swicloud.atlassian.net/browse/NH-72398
There was a problem hiding this comment.
LGTM, really appreciate the PR description and example data as always @tammy-baylis-swi! Left a few comments.
solarwinds_apm/sampler.py
Outdated
| 1. SW_APM_TRANSACTION_NAME | ||
| 2. AWS_LAMBDA_FUNCTION_NAME | ||
| 3. automated naming from span name | ||
| 4. "unknown_service" backup, to match OpenTelemetry SDK Resource default |
There was a problem hiding this comment.
I missed adding this to our confluence spec (just did), but the last resort default value should be "unknown"--it's the existing transaction naming logic in the core library so let's keep with it for lambda.
There was a problem hiding this comment.
Also, the transaction name should be truncated to 255 chars--this is needed when we export the response time metric via OTLP since the core library is not involved (and thus truncate as it does for the AO proto metrics). Hmm, not sure if Java and Node.js actually conform to this part of the spec currently cc @cleverchuk @raphael-theriault-swi please check.
There was a problem hiding this comment.
Nope Node doesn't do it at the moment, thanks for pointing this out
| @@ -415,6 +419,35 @@ def add_tracestate_capture_to_attributes_dict( | |||
| ) | |||
| return attributes_dict | |||
|
|
|||
| def calculate_otlp_transaction_name( | |||
There was a problem hiding this comment.
Hmm yes good point, it would be nice to have it shared, although W3C... is very much an HTTP header concern while transaction naming is our SWO-specific concept, so I wouldn't put it there.
There is the top-level apm_txnname_manager, and some other transaction name classes under trace/ that might fit the bill. But you may want to just defer the refactoring until you've had a chance to investigate the idea of subclassing the OTel exporter in order to support custom SDK naming for lambda (which can definitely be a future iteration).
|
Thank you both! |
Updates Sampler to add
sw.transactionattribute to spans, ifexperimental("OTLP export") flag is set.Dumbs down OTLPMetricsSpanProcessor to match
sw.transactionattribute in OTLP metrics.We can only customize transaction value for spans with the env var
SW_APM_TRANSACTION_NAMEorAWS_LAMBDA_FUNCTION_NAME, not with the SDK (see Confluence). For now, the latter env var we assume is always there. Backups in case not present are the span's name, else "unknown_service" like Resource in the SDK.Tests from a mocked lambda environment and requested route with a 3s sleep as follows.
SW_APM_TRANSACTION_NAME: my-cool-txnandAWS_LAMBDA_FUNCTION_NAME: flask-mock-lambda:AWS_LAMBDA_FUNCTION_NAME: flask-mock-lambda:Please let me know what you think.