Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add instrumentation API and native OpenTelemetry implementation #588

Merged
merged 9 commits into from
Aug 21, 2023

Conversation

AlexanderWert
Copy link
Member

@AlexanderWert AlexanderWert commented May 27, 2023

Adds an instrumentation API with request & response lifecycle callbacks, and an implementation based on OpenTelemetry that is used by default.

To be able to implement the OpenTelemetry semantic conventions, this PR also adds a pathParameters method to Endpoint that provides access to the values in URL template placeholders.

@AlexanderWert AlexanderWert force-pushed the otel-instrumentation branch 2 times, most recently from 19a0531 to 73cab12 Compare May 28, 2023 09:22
Copy link
Member

@swallez swallez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Overall it looks nice.

Rather than the route (i.e. path template), using the API endpoint identifier would make things easier. This identifier comes from the API specification (see e.g. IndexRequest) and is available as endpoint.id().

Benefits:

  • a single endpoint can have several routes and different http methods depending on which optional path parameters are set,
  • using endpoint id avoids doing some pattern matching on the route for things like document id extraction.
  • naming the span with the endpoint id ensures a 1:1 mapping from APIs to span names regardless of the path parameters set.

Note: I suggested this to @estolfo in a recent discussion.

@AlexanderWert
Copy link
Member Author

Thanks! Overall it looks nice.

Rather than the route (i.e. path template), using the API endpoint identifier would make things easier. This identifier comes from the API specification (see e.g. IndexRequest) and is available as endpoint.id().

Benefits:

  • a single endpoint can have several routes and different http methods depending on which optional path parameters are set,
  • using endpoint id avoids doing some pattern matching on the route for things like document id extraction.
  • naming the span with the endpoint id ensures a 1:1 mapping from APIs to span names regardless of the path parameters set.

Note: I suggested this to @estolfo in a recent discussion.

I agree that we could improve some of the internal detection of request types by using the endpoint ID. I'll change that to make it more efficient and reliable.

However, from an APM point of view, we would like to be able to differentiate the different patterns even within a single endpoint. For instance, we'd like to have different span names for requests to /_search vs. /{index}/_search.
Also from a user perspective it seems more natural to show the actual pattern being used instead of abstracting it away through a request name.

Regarding regex matching:

  • it's not required in the native instrumentation anyways
  • for the auto-instrumentation in the agent (for older client versions) we would still need to do it because we need to extract the index and doc_id as we provide this as explicit attributes.

So my suggestion would be to use the endpoint id for internal optimization of the instrumentation but still exposing the actual pattern as the span names.

WDYT?

\cc @estolfo

@AlexanderWert
Copy link
Member Author

@swallez

As open-telemetry/semantic-conventions#23 has been merged, I updated this PR accordingly and also incorporated your suggestions.
I think it's safe now to finalize this PR.

@swallez swallez marked this pull request as ready for review August 21, 2023 14:31
@swallez swallez changed the title Added native instrumentation using OpenTelemetry API Add instrumentation API and native OpenTelemetry implementation Aug 21, 2023
Copy link
Member

@swallez swallez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@swallez swallez merged commit 6c49b12 into elastic:main Aug 21, 2023
4 checks passed
swallez added a commit that referenced this pull request Aug 21, 2023
swallez added a commit that referenced this pull request Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants