Enforce parent-child relationship for ES/HTTP span #1963
Conversation
github-actions
bot
added
the
agent-nodejs
|
Interesting. Compare to https://github.com/elastic/apm-agent-nodejs/pull/1890/files FWIW, which is my proposed patch for solving this general issue for all cases -- albeit with the requirement to grasp possible impact of that wider behaviour change. |
💔 Tests Failed
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪Test errors
Expand to view the tests failures> Show only the first 10 test failures
|
|
@trentm I've moved this out of draft so we can make a decision at some point. Feel free to close it if you feel it's not the right approach. |
|
jenkins run the tests Trying the jenkins run again to rule out a transient. |
|
jenkins run the tests please |
Ah, do you hit the same issue with custom spans? Something like: // ...
const span = apm.createSpan("doingThatThing")
await esClient.makeACall(...)
// ...
span.end()Obviously that underscore in
|
|
@trentm Hmm. Let me rephrase: if we have to pick between |
I don't know what I was thinking when I said that. This is a patch to the agent itself.
Promise.resolve - you can do without the agent needing to take this change or to wait for #1890 to do similar. I don't have any idea the potential impact on Kibana perf with it doing that in (all?) its calls to ES. I do want to get to these changes, but I probably won't get there quickly. What if the agent had an option to elide the child HTTP span for ES queries altogether? (I'm assuming we can even do this relatively easily in the node agent.) Would that even be preferable for your case? It would effectively workaround this issue until parent-child spans in general are tackled. |
I'm currently using the awaiting
I'm not sure what the consequences would be there. I do like the idea of less spans, but the spans might have different semantics which might be useful for a specific scenario (e.g., the HTTP span might record the network error, something like that?) I'm okay with postponing this for sure. What also might be an option is to have the agent add something like a |
|
Hey guys, any progress on that one ? I'm started to get tired of all these double HTTP/Elasticsearch spans 😊 Thx, |
|
@ddolcimascolo hello David, that's being tracked in this issue I think: #2000 |
|
@ddolcimascolo I'm currently looking into #2000 (and effectively this ticket as well). Nothing to report yet. |
|
Thx guys |
|
I believe this is fixed by #2181 |
All instrumented calls from the Elasticsearch client create two spans: first, an
Elasticsearchspan, and then, anhttpspan. Currently, thehttpspan is a sibling of theElasticsearch, rather than its child, because thehttpspan is created in the same async operation. This leads to breakdown timings being counted twice and makes it hard to see parent/child relationships in the trace waterfall.As addressing this in a general manner has bigger consequences, I've tried to address this specifically for the ES client's instrumentation where the scope is relatively small and known.
I'm also wondering if we can use this strategy in our custom instrumentation in Kibana (instead of awaiting process.nextTick or Promise.resolve.