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

use semantic convention APIs for opentelemetry #2662

Merged
merged 7 commits into from
Aug 17, 2023

Conversation

aleqi200
Copy link
Contributor

@aleqi200 aleqi200 commented Jul 25, 2023

Motivation:

Today the tags are manually handcrafted with strings, if the opentelemetry api changes, the tags will be outdated, using the instrumenter available from the io.opentelemetry.instrumentation:opentelemetry-instrumentation-api-semconv we can make it all dynamic based on the specification without understanding the specification and its tags.

Result:

API will become more stable and evolve easier together with opentelemetry, no more handcrafted strings and the conventions will be driven by the instrumentation API.

@aleqi200 aleqi200 force-pushed the semantic_otel_http branch 3 times, most recently from 1848712 to 8254ce3 Compare July 27, 2023 21:10
@aleqi200
Copy link
Contributor Author

The tags are now showing the following:
Screenshot 2023-07-27 at 2 20 12 PM
The tags are driven by the semantic conventions available in the API.

Tag Value
http.method POST
http.request.header.transaction_id [ "1234" ]
http.request.header.x_shield_req_id [ "1234-asdf" ]
http.request_content_length 87
http.response_content_length 36
http.route /job
http.status_code 200
http.target /job
internal.span.format proto
net.host.name localhost
net.host.port 8082
net.protocol.name http
net.protocol.version 1.1
otel.library.name io.servicetalk
otel.status_code OK
span.kind server
thread.id 24
thread.name servicetalk-global-io-executor-1-3
user_agent.original curl/7.88.1

This will allow capture of request/response headers like above http.request.header.transaction_id.

@aleqi200
Copy link
Contributor Author

Metrics will be available for spans as well.
The following metrics were captured using the updated API with opentelemetry and prometheus:
Screenshot 2023-07-26 at 9 50 46 AM

Motivation:

Today the tags are manually handcrafted with strings, if the opentelemetry api changes, the tags will be outdated, using the instrumenter available from the io.opentelemetry.instrumentation:opentelemetry-instrumentation-api-semconv we can make it all dynamic based on the specification without understanding the specification and its tags.

Result:

API will become more stable and evolve easier together with opentelemetry, no more handcrafted strings and the conventions will be driven by the instrumentation API.
gradle.properties Outdated Show resolved Hide resolved
@@ -52,21 +59,62 @@
*/
public final class OpenTelemetryHttpServerFilter extends AbstractOpenTelemetryFilter
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should rename this to OpenTelemetryHttpServiceFilter to have proper alignment with the client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renaming will break clients who are already using it.

Copy link
Member

Choose a reason for hiding this comment

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

We can introduce new names and deprecate all names (not in this PR, in a follow-up)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will do in a follow up PR

@@ -54,17 +63,42 @@
public final class OpenTelemetryHttpRequestFilter extends AbstractOpenTelemetryFilter
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should rename this to OpenTelemetryHttpClientFilter to have proper alignment with the service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's too late. We cannot break the contract with clients, renaming will cause a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

Not in this PR, but in a follow up it will be nice to rename it to OpenTelemetryHttpRequesterFilter to be consistent with all other filters. We use "requester" on the client-side because all clients and connections implement HttpRequester interface.

For backward compatibility, we can deprecate current OpenTelemetryHttpRequestFilter and link to OpenTelemetryHttpRequesterFilter as a replacement.

@@ -52,21 +59,62 @@
*/
public final class OpenTelemetryHttpServerFilter extends AbstractOpenTelemetryFilter
Copy link
Member

Choose a reason for hiding this comment

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

We can introduce new names and deprecate all names (not in this PR, in a follow-up)

@@ -54,17 +63,42 @@
public final class OpenTelemetryHttpRequestFilter extends AbstractOpenTelemetryFilter
Copy link
Member

Choose a reason for hiding this comment

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

Not in this PR, but in a follow up it will be nice to rename it to OpenTelemetryHttpRequesterFilter to be consistent with all other filters. We use "requester" on the client-side because all clients and connections implement HttpRequester interface.

For backward compatibility, we can deprecate current OpenTelemetryHttpRequestFilter and link to OpenTelemetryHttpRequesterFilter as a replacement.

Copy link
Member

@idelpivnitskiy idelpivnitskiy left a comment

Choose a reason for hiding this comment

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

Pushed 2 commits to address the last comments I have below:

@aleqi200
Copy link
Contributor Author

the changes look good. I made a test and it looks good.

Copy link
Member

@idelpivnitskiy idelpivnitskiy left a comment

Choose a reason for hiding this comment

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

Thank you!

@idelpivnitskiy idelpivnitskiy merged commit 4837cd8 into apple:main Aug 17, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants