Add message attributes to transactions and spans#3104
Conversation
Add attributes holding messaging information to events to support monitoring message systems. closes elastic#3006
|
LGTM, but I am not fully aware of the implications of storage decisions.
Do you do the same with HTTP spans/transactions sent without request info? Another example is that only Lastly- you should be aware that |
The check was added based on the comment in elastic/apm#143 (comment): context.message.queue.name and context.message.topic.name: required for messaging spans and transactions. Will be used for the service map. Indexed as keyword.
No we don't. But according to the description I referred to above, a message information is required when the event type is According to @eyalkoren 's comment above, I will change the json spec requirement to only require either
My reasoning was that one might want to filter by |
Sorry, that was long ago, updated it. New dedicated I am really fine with any decision based on storage/query convenience, my comments are only for making sure you are aware of what they mean from the agent perspective. |
|
From the updated comment:
@eyalkoren I changed the Intake API to either require |
|
@simitt right, didn't update that 🤦♂ |
|
@eyalkoren are the values for By storing the messaging related information under |
No, I use Honestly, I wouldn't drive any schema decisions if the only query it is required for is the |
* Store message information under event type, e.g. `span.message`. * Remove messaging type check * Make all message fields optional. * fix tests * Remove topic.name
|
After discussions with @eyalkoren and @sqren I made following changes:
There are some uncertainties about how the message information should be used from an UI perspective and mid- to longterm plans with that. Maybe @eyalkoren or @nehaduggal can bring more clarification into this from a product view, to avoid driving the discussion from an implementation viewpoint (might be better to clarify and move this discussion to elastic/kibana#49465 or elastic/apm#143). |
Codecov Report
@@ Coverage Diff @@
## master #3104 +/- ##
=========================================
Coverage ? 78.44%
=========================================
Files ? 98
Lines ? 4959
Branches ? 0
=========================================
Hits ? 3890
Misses ? 1069
Partials ? 0
|
|
According to elastic/kibana#49465 (comment) it seems the UI team is fine with the suggested ES storage. |
axw
left a comment
There was a problem hiding this comment.
Mostly LGTM, just a few minor things
beater/test_approved_es_documents/TestPublishIntegrationSpans.approved.json
Outdated
Show resolved
Hide resolved
Co-Authored-By: Andrew Wilkins <axwalk@gmail.com>
Add attributes holding messaging information to transactions and spans to support monitoring message systems. closes elastic#3006
Add attributes holding messaging information to transactions and spans to support monitoring message systems. closes elastic#3006
| case http.Header: | ||
| if value != nil { | ||
| m[key] = value | ||
| } else if remove { | ||
| delete(m, key) | ||
| } |
There was a problem hiding this comment.
How would you see joining cases that are exactly equal?
case *bool, *int:
if value != nil {
m[key] = *value
} else if remove {
delete(m, key)
}Another nit: all the else if branches that only check for (val == nil && remove) (6 occurrences) are already covered by L:47-52:
if val == nil {
if remove {
delete(m, key)
}
return
}There was a problem hiding this comment.
all the else if branches that only check for (val == nil && remove) (6 occurrences) are already covered by L:47-52:
The checks are necessary as a non-nil interface can point to a nil value, see https://play.golang.com/p/nhsiTGDhg7V
There was a problem hiding this comment.
Ahhh sorry, I committed a mistake when reading the code: the internal evaluation is for value, not for val, so it's checking the type (switch value := val.(type))
Thanks for the clarification!
Add attributes holding messaging information to events to support monitoring message systems.
Intake API:
All message attributes defined in elastic/apm#143 (comment) are added to the Intake API:
message.queue.name(string, maxLength 1024,required)message.topic.name(string, maxLength 1024, required)message.age.ms(integer, optional)message.body(string, optional)message.headers(object with string or array properties, optional)There is no distinction between
transactionandspanmessages as there is no obvious advantage of separate handling, while a distinction would duplicate certain fields and therefore be more error prone.Ifspan.type == messaging || transaction.type == messagingand no message information is sent an error will be thrown. The check for this is not in the json spec but in the go code.Storage ES
Since ECS already defines a root levelmessagefield astext, a root level objectmessagingis introduced. To allow aggregations and easy queries over message fields for spans and transactions at the same time, the information is not stored under the event typetransactionandspanbut under a root level keymessaging. This objectmessagingcontains atypedefining the messaging system, e.g.JMS, rabbitmq, etc. and the actualmessage.Forspansthespan.subtypeis copied tomessaging.type, fortransactionsthis field is unknown.Forspansthespan.actionis copied tomessaging.message.operation, fortransactionsthis field is unknown.The fields are copied for easier queries when this information is needed.Since there is no immediate use case for aggregating messaging related information, the information is stored under the event type, to avoid introducing a non-ECS root field.
Information is stored as
transaction.messageandspan.message.Following fields are indexed:
messaging.typemessaging.message.queue.namemessaging.message.topic.namemessaging.message.age.msmessaging.message.operationspan.message.queue.namespan.message.age.mstransaction.message.queue.nametransaction.message.age.msMessage Headers
The message headers are internally treated the same way as http headers, which means that they are canonicalized before stored to Elasticsearch.
implements #2697
@eyalkoren please let me know if this makes sense to you.