Add message attributes to transactions and spans

[simitt]

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 transaction and span messages as there is no obvious advantage of separate handling, while a distinction would duplicate certain fields and therefore be more error prone.

If span.type == messaging || transaction.type == messaging and 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 level message field as text, a root level object messaging is 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 type transaction and span but under a root level key messaging. This object messaging contains a type defining the messaging system, e.g. JMS, rabbitmq, etc. and the actual message.

For spans the span.subtype is copied to messaging.type, for transactions this field is unknown.
For spans the span.action is copied to messaging.message.operation, for transactions this 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.message and span.message.

Following fields are indexed:

• messaging.type
• messaging.message.queue.name
• messaging.message.topic.name
• messaging.message.age.ms
• messaging.message.operation
• span.message.queue.name
• span.message.age.ms
• transaction.message.queue.name
• transaction.message.age.ms

Message 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.

[eyalkoren]

LGTM, but I am not fully aware of the implications of storage decisions.
The best input I can provide is that it can be useful to look at the relationships between messaging spans and transactions as (mostly) equivalent to the relationships between inbound (Transaction) and outbound (Span) HTTP requests.
For example:

If span.type == messaging || transaction.type == messaging and 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.

Do you do the same with HTTP spans/transactions sent without request info?
Do you also store an equivalent to messaging.type for HTTP (eg the name of an HTTP client related to an HTTP span)? What would it be used for?

Another example is that only messaging transactions can contain header or body info, similar to HTTP transactions vs. spans, if that makes any difference.

Lastly- you should be aware that message.queue.name and message.topic.name should not be sent for the same message. It is either one or the other. The reason we decided to have both is that either would make sense for different systems/scenarios.

[simitt]

If span.type == messaging || transaction.type == messaging and 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.

Do you do the same with HTTP spans/transactions sent without request info?

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.

Do you do the same with HTTP spans/transactions sent without request info?

No we don't. But according to the description I referred to above, a message information is required when the event type is messaging. I can loosen the requirement, but the UI and service map logic need to be able to handle missing messaging information then.
@eyalkoren I am fine either way; @elastic/apm-ui can you let us know if having an event.type=messaging without any messaging information would be problematic?

According to @eyalkoren 's comment above, I will change the json spec requirement to only require either message.queue.name or message.topic.name when a message is sent.

Do you also store an equivalent to messaging.type for HTTP (eg the name of an HTTP client related to an HTTP span)? What would it be used for?

My reasoning was that one might want to filter by messaging.type, e.g. when using JMS but also something else like rabbitmq. This was mainly thought for querying convenience for the UI. But if that field doesn't seem important I can remove it. @elastic/apm-ui maybe we should do a quick sync on how the data will be used.

[eyalkoren]

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.

Sorry, that was long ago, updated it. New dedicated destination.service fields were added for the service map. Those are nested within the top context level, so you can enforce their existence for the purpose of service maps as you would do with others.

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.

[simitt]

From the updated comment:

context.message.queue.name and context.message.topic.name: required for messaging spans and transactions. Indexed as keyword.

@eyalkoren I changed the Intake API to either require context.message.queue.name OR context.message.topic.name to be present if context.message.* information is sent. Is this how it should be or should none of this information be required?

[eyalkoren]

@simitt right, didn't update that 🤦‍♂
Updated now- let's make it optional. The reason is that there are cases where we trace queue/topic polling actions that happen within a traced transaction, and those may be reported without a queue/topic.

[simitt]

@eyalkoren are the values for span.action fixed to receive and send? I currently copy the span.action to the messaging.message.operation field, and think it would make sense to also set it for transaction events.

By storing the messaging related information under messaging instead of event_type.message and duplicating the action, queries can be simplified.
E.g. to query for all incoming messages for one trace the query would be trace_id=xyz AND messaging.message.operation=receive rather than trace_id=xyz AND (span.type=messaging AND span.action=receive OR transaction.type=messaging).

[eyalkoren]

are the values for span.action fixed to receive and send?

No, I use send and poll for Kafka... It may still make sense to use receive for this purpose, as poll wouldn't have the age filed, only receive spans and transactions.

Honestly, I wouldn't drive any schema decisions if the only query it is required for is the age field. This is a somewhat experimental/evaluation info that was recently added and I prefer we can get some milage with it and evaluate it.

[simitt]

After discussions with @eyalkoren and @sqren I made following changes:

• store message information under event type, e.g. transaction.message and span.message. There is no use case planned for aggregating on the message information and it is also not part of service maps. The only currently planned query is for visualizing message information in the trace metadata. Therefore introducing a dedicated (non ECS) root field for potential future query or aggregation use cases seems premature.
• do not require any message fields on the Intake API
• remove check that message information is sent when span.type=messaging or transaction.type=messaging, as there might be valid use cases for this.
• remove context.message.topic.name, the information will be sent via context.message.queue.name.
• fix system tests

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-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@7509601). Click here to learn what that means.
The diff coverage is 95.23%.

@@            Coverage Diff            @@
##             master    #3104   +/-   ##
=========================================
  Coverage          ?   78.44%           
=========================================
  Files             ?       98           
  Lines             ?     4959           
  Branches          ?        0           
=========================================
  Hits              ?     3890           
  Misses            ?     1069           
  Partials          ?        0

Impacted Files	Coverage Δ
tests/json_schema.go	21.16% <0%> (ø)
model/stacktrace_frame.go	100% <100%> (ø)
model/span/event.go	85.58% <100%> (ø)
model/error/event.go	97.51% <100%> (ø)

[simitt]

According to elastic/kibana#49465 (comment) it seems the UI team is fine with the suggested ES storage.

[axw]

Mostly LGTM, just a few minor things

[mdelapenya]

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
	}

[simitt]

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

[mdelapenya]

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!