-
Notifications
You must be signed in to change notification settings - Fork 26
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
Detect messaging.{system,operation} in TranslateTransaction #204
Detect messaging.{system,operation} in TranslateTransaction #204
Conversation
expectedLabels: map[string]*modelpb.LabelValue{ | ||
"messaging_system": {Value: "kafka"}, | ||
}, | ||
expectedTxnMessage: nil, |
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.
[For reviewer] As demonstrated by the test case, this is a new case that doesn't exist before. When messaging.system
or messaging.operation
is present AND messaging.destination
is absent, do we want event.Transaction.Message to be empty or nil? Does it make a difference?
To rephrase the question, is it ok to have event.Transaction.Message == nil
when event.Transaction.Type == "messaging"
?
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; this PR fixes the bug that the transaction type is not set although messaging attributes are sent.
However, the apm message model is pretty outdated compared to the otel messaging spec. So my suggestion would be to open a follow up github issue for updating the supported semconv version and then also update the messaging model definition. I'd see it outside the scope of this PR though.
input/otlp/traces.go
Outdated
@@ -411,8 +410,17 @@ func TranslateTransaction( | |||
|
|||
// messaging.* | |||
case "message_bus.destination", semconv.AttributeMessagingDestination: | |||
message.QueueName = stringval | |||
if event.Transaction.Message == nil { | |||
event.Transaction.Message = modelpb.MessageFromVTPool() |
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.
[For reviewer] Note: IMO there are still quite a few cases where we are doing allocs in this function, when we should use vt pool instead. This change fixes one.
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 saw the improvement - it would make sense to open a tech debt issue, collecting places where allocations could be reduced.
@@ -495,7 +501,12 @@ func TranslateTransaction( | |||
event.Url = modelpb.ParseURL(httpURL, httpHost, httpScheme) | |||
} | |||
if isMessaging { | |||
event.Transaction.Message = &message | |||
// Overwrite existing event.Transaction.Message |
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.
Moved this code back here. It is important to overwrite existing event.Transaction.Message to avoid breaking otel bridge test when event.Transaction.Message and the otel attribute exist at the same time.
=== RUN TestDecodeMapToTransactionModel/otel-bridge/messaging
transaction_test.go:559:
Error Trace: /home/carson/projects/apm-data/input/elasticapm/internal/modeldecoder/v2/transaction_test.go:559
Error: Not equal:
expected: &modelpb.Message{state:impl.MessageState{NoUnkeyedLiterals:pragma.NoUnkeyedLiterals{}, DoNotCompare:pragma.DoNotCompare{}, DoNotCopy:pragma.DoNotCopy{}, atomicMessageInfo:(*impl.MessageInfo)(nil)}, sizeCache:0, unknownFields:[]uint8(nil), Body:"", Headers:[]*modelpb.HTTPHeader(nil), AgeMillis:(*uint64)(nil), QueueName:"myQueue", RoutingKey:""}
actual : &modelpb.Message{state:impl.MessageState{NoUnkeyedLiterals:pragma.NoUnkeyedLiterals{}, DoNotCompare:pragma.DoNotCompare{}, DoNotCopy:pragma.DoNotCopy{}, atomicMessageInfo:(*impl.MessageInfo)(nil)}, sizeCache:0, unknownFields:[]uint8(nil), Body:"init", Headers:[]*modelpb.HTTPHeader{(*modelpb.HTTPHeader)(0xc00040aa50)}, AgeMillis:(*uint64)(0xc000327058), QueueName:"myQueue", RoutingKey:"init"}
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 how the message instance would already be non-nil at this point for otel data, so it should be fine.
@@ -495,7 +501,12 @@ func TranslateTransaction( | |||
event.Url = modelpb.ParseURL(httpURL, httpHost, httpScheme) | |||
} | |||
if isMessaging { | |||
event.Transaction.Message = &message | |||
// Overwrite existing event.Transaction.Message |
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 how the message instance would already be non-nil at this point for otel data, so it should be fine.
Use
messaging.system
andmessaging.operation
to detect OTel messaging span in TranslateTransaction. The corresponding handling already exists in TranslateSpan.Closes #194