exclude the HTTP span under an Elasticsearch span

[trentm]

From discussion on today's apm-agent-nodejs call and from elastic/apm#420 we think the HTTP span "under" an Elasticsearch (ES) span from instrumentation of the "elasticsearch" and "@elastic/elasticsearch" modules should be excluded. The ES span is an "exit" span, and arguably the HTTP request it makes is an implementation detail. As well, eliding that span would make breakdown metrics much more useful/realistic. (FWIW, it would also side-step the current issue with the HTTP span being a sibling of, rather than a child of, the ES span for the instrumentation of the newer "@elastic/elasticsearch" client.)

From memory (though might be nice to confirm), the Python agent excludes the HTTP span (via leaf=True in the code). I recall one other language agent optionally including the HTTP span, but I think it was off by default.

[trentm]

This was discussed on the APM Agents call yesterday. The pretty strong consensus seemed to be to not emit HTTP spans (or whatever other transport span) underneath higher-level instrumentation spans: e.g. no HTTP span underneath an SQS span, no HTTP span underneath an Elasticsearch span.

Also this note from eyal:

Still, as @axw mentioned, there may be value in keeping lower-span-data (like HTTP status code or URL) when you have such, and the way we chose doing that for ES client spans is by populating context.http fields, even though the span type is not HTTP.

[dgieselaar]

How do we capture retries if the HTTP span is dropped?

[joshdover]

+1 on this for Kibana's usage. We'd like to start enabling our support team to use APM instrumentation to debug performance problems and I think this duplication creates confusing noise.

How do we capture retries if the HTTP span is dropped?

Could we add a context.http.attempts field?

[trentm]

How do we capture retries if the HTTP span is dropped?

I'll ask the APM group. I don't see any current apm-server intake fields, no ECS http fields that could easily hold this value.

[trentm]

Note to self: because a span synchronously created under a current span (e.g. the HTTP span under an ES span) currently isn't set as a child might complicate implementation of this. If we had the child relationship (as we eventually want), then the implementation could just be something like the Python agent's span.leaf=true and use that to avoid creating new span children.

[trentm]

Now that #2181 is in we have the parent/child relationship we need. Implementing this (and #2125) should be via implementing exit span support and marking ES spans as exit spans.

Need to make sure context propagation still works. I'm not sure if that'll be via having the ES instrumentation add the trace-context headers, or by keeping that in HTTP instrumentation, even though a span isn't created there. I think probably the latter. The handling in http-shared.js already handles propagating trace-context headers even if span === null (e.g. from hitting transaction_max_spans or other conditions).

Also, perhaps think ahead about compressed span handling implications.

[trentm]

Note that I haven't pursued the "how to track retries". We discussed it at an APM Agents call (2022-02-01). There isn't a currently suitable ECS field for this. I considered adding a span label for this, but it would be first case of any Node.js APM agent instrumentation adding span labels automatically -- I'm not sure if span labels should remain the domain of the user.

User code (e.g. Kibana code) should be able to add an ES Client "response" handler to add a label to the apm.currentSpan with the number of attempts. Something like:

client.on('response', (_err, result) => {
  // Obviously missing defensive guards here.
  apm.currentSpan.setLabel('attempts', result.meta.attempts, false)
})