[APM][Otel] Errors: Add fallback to span id if the parent id is undefined
#195796
Conversation
jennypavlova
added
release_note:skip
|
Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services) |
botelastic
bot
added
the
ci:project-deploy-observability
|
@jennypavlova the other direction (from error to trace) is also broken, I see you covered the Waterfall here, would we need to cover the error detail view as well? The link pointed out in the following screenshot does not appear for OTel-native errors: OTel Native:
|
💔 Build Failed
Failed CI Steps
Test Failures
Metrics [docs]Async chunks
History |
| | IWaterfallSpanOrTransaction | ||
| | undefined; | ||
| const parent = items.find( | ||
| (waterfallItem) => waterfallItem.id === (error.parent?.id ?? error.span?.id) |
There was a problem hiding this comment.
@jennypavlova have you considered moving this logic to the server instead of handling it on the client? I ask that because we'll probably have many hidden problems if we support multiple schemas on the client. As @AlexanderWert has already pointed out there's another place where the same logic is needed.
I wonder if the UI should be schema-agnostic and respect its own schema.
This is similar to a draft PR @rmyz opened recently #194100 (review).
There was a problem hiding this comment.
Thanks for the comment, @cauemarcondes
In general, I agree with you but this is just a fallback to a different field if the parent id is not there, I wouldn't call it a 'new' schema. It can be confusing if we modify the response and return the parent.id in a document that doesn't have it but it probably helps us in the long run as we will have the parent id available in all places in the UI. I remember that we discussed with @dgieselaar and @gregkalapos that we wanted to do a fallback on the client to keep it compatible and return the fields in the same format as they come from elasticsearch in the server.
But we can do it on the server, just need the changes made in #195242 as a base and merge a PR there
@AlexanderWert has already #195796 (comment) there's another place where the same logic is needed.
This is unrelated. I investigated more and this happens because the transaction.id is missing and the query and it is not returning the transaction object - even if I do what you suggested it won't fix the issue mentioned there
It's a different query (changed here) I am not sure why we are missing the transaction id in the error document - I see that also span id is not there (using the test env)
There was a problem hiding this comment.
@jennypavlova ahhh right, great finding!
I think it's not that easy (maybe impossible) to actually have the transaction.id on OTel errors, because it cannot be enriched from other attributes but would be required to be collected by the SDKs / APM Agents. That's a conceptual limitation. So, then I think let's proceed with this PR as is.
\cc @gregkalapos @felixbarny correct me if I'm wrong with the above
|
@cauemarcondes @AlexanderWert I created a PR to Carlos' PR because I need the changes there as I followed the suggestion to add the fallback changes in the query response cc: @crespocarlos I think only you can review/approve it |
|
Closing in favor of crespocarlos#4 |
Closes #195731
Summary
This PR fixes a bug with error correlations not displayed correctly in the APM waterfall when using Otel native data. In Otel native data
parent.idis never defined so in this case we need to fallback tospan.idTesting
Warning
The changes of #195242 are required to properly test this. so there is a unit test to cover that before the PR is merged
Using the e2e PoC: navigate to a service overview - > Transactions and scroll to the waterfall: