elastic / apm

Elastic Application Performance Monitoring - resources and general issue tracking for Elastic APM.
https://www.elastic.co/apm
Apache License 2.0
384 stars 114 forks source link

OTel bridge require null transaction result #649

Closed SylvainJuge closed 2 years ago

SylvainJuge commented 2 years ago

During the investigation of https://github.com/elastic/apm-server/issues/8067 we found out that the behavior of the apm-server does not infer any value for transaction.result if a value is already set.

As a result, when an agent sets the transaction.result value in the OTel bridge, setting any of the usual attribute http.status_code = 200 does not produce a transaction that has transaction.result = HTTP 2xx as would have been expected.

This is mostly a bugfix in the spec and very likely not controversial nor require further discussion. Only agents that do rely on the OTel bridge gherkin spec should be directly impacted, but checking the behavior in all bridge implementations would still be required.

apmmachine commented 2 years ago

:green_heart: Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

#### Build stats * Start Time: 2022-07-08T05:31:00.797+0000 * Duration: 6 min 58 sec

trentm commented 2 years ago

The Node.js APM agent will have to update for this. Currently (with the Node.js agent) the default transaction.result for all transactions (whether from the OTel Bridge or not) default to "success". For the OTel Bridge work I had copied the Java agent behaviour to set transaction.result when OTel's span.setStatus(...) was called: https://github.com/elastic/apm-agent-nodejs/blob/7b2dddf59b736d829c9de65442fdb46eae65e44b/lib/opentelemetry-bridge/OTelSpan.js#L127-L137

I can remove that block, but that will still leave the default transaction.result === "success".

SylvainJuge commented 2 years ago

@trentm this needs to be changed only for OTel-bridge-created transactions, not for all transactions created by the agents.

For the general case of default values for transaction.result, I don't think it is really well defined (yet), in the Java agent we have the following values:

Which means it could definitely be a good topic for a follow-up PR in specs.

SylvainJuge commented 2 years ago

Apart from the spec change, only the @elastic/apm-agent-net might be directly impacted by this change as it is relying on the gherkin specs for OTel bridge. The impact of this change is very minimal anyway, so I will be merging this within next week if there is no further feedback against it.