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

lambda: change context.service.origin.name spec to not re-order 'stage' position in path #543

Closed trentm closed 3 years ago

trentm commented 3 years ago

This was a change that I'd asked about in https://github.com/elastic/apm/pull/542 discussion, but I merged that PR before adding the suggested change.

Repeating my question/suggestion from the earlier PR:

The [current] spec [for context.service.origin.name] is appending /$stage. Is there a particular reason to do it that way? The reason I ask is because I think this could be misleading/confusing. When I create an API Gateway trigger for function name myfunc and stage mystage, the API endpoint is something like: https://cmfuqmfy79.execute-api.us-west-2.amazonaws.com/mystage/myfunc. We are reporting the service.origin.name as GET /myfunc/mystage which reverses the order of those fields.

Unless there is a particular reason for the existing spec, I'd suggest we use the order in event.requestContext[.http].path, e.g. /mystage/myfunc.

The (long) rendered table cell is viewable here: https://github.com/elastic/apm/blob/lambda-clarifications2/specs/agents/tracing-instrumentation-aws-lambda.md#api-gateway-v1--v2

apmmachine commented 3 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: 2021-11-08T19:31:42.981+0000 * Duration: 4 min 40 sec * Commit: 0509dcdb64844b108906b6949306ee765f91f348

eyalkoren commented 3 years ago

I am applying this change now for the Java agent, just want to clarify: we decided to ignore the stage when constructing context.service.origin.name because it should already be included in the path, is that correct? Because from @basepi's comment, it can be understood as if the intention is to reverse the order of stage and path when constructing the name.

trentm commented 3 years ago

we decided to ignore the stage when constructing context.service.origin.name because it should already be included in the path, is that correct?

Correct.