-
Notifications
You must be signed in to change notification settings - Fork 243
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
Conversation
19a0531
to
73cab12
Compare
There was a problem hiding this 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.
java-client/src/main/java/co/elastic/clients/transport/Endpoint.java
Outdated
Show resolved
Hide resolved
java-client/src/main/java/co/elastic/clients/transport/rest_client/InstrumentationUtil.java
Outdated
Show resolved
Hide resolved
java-client/src/main/java/co/elastic/clients/transport/rest_client/InstrumentationUtil.java
Outdated
Show resolved
Hide resolved
java-client/src/main/java/co/elastic/clients/transport/rest_client/InstrumentationUtil.java
Outdated
Show resolved
Hide resolved
java-client/src/main/java/co/elastic/clients/transport/rest_client/InstrumentationUtil.java
Outdated
Show resolved
Hide resolved
java-client/src/main/java/co/elastic/clients/transport/rest_client/InstrumentationUtil.java
Outdated
Show resolved
Hide resolved
java-client/src/main/java/co/elastic/clients/transport/rest_client/InstrumentationUtil.java
Outdated
Show resolved
Hide resolved
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 Regarding regex matching:
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 |
d81f7dc
to
7f85f7f
Compare
As open-telemetry/semantic-conventions#23 has been merged, I updated this PR accordingly and also incorporated your suggestions. |
Signed-off-by: Alexander Wert <[email protected]>
Signed-off-by: Alexander Wert <[email protected]>
Signed-off-by: Alexander Wert <[email protected]>
Signed-off-by: Alexander Wert <[email protected]>
Signed-off-by: Alexander Wert <[email protected]>
14e43a5
to
b49348c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Alexander Wert <[email protected]> Co-authored-by: Sylvain Wallez <[email protected]>
#652) Signed-off-by: Alexander Wert <[email protected]> Co-authored-by: Alexander Wert <[email protected]>
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 toEndpoint
that provides access to the values in URL template placeholders.