-
Notifications
You must be signed in to change notification settings - Fork 867
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
Extract HTTP request/response headers as span attributes #4237
Extract HTTP request/response headers as span attributes #4237
Conversation
...rc/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/CapturedHttpHeaders.java
Show resolved
Hide resolved
...va/io/opentelemetry/instrumentation/api/instrumenter/http/HttpCommonAttributesExtractor.java
Show resolved
Hide resolved
CLIENT = | ||
CapturedHttpHeaders.create( | ||
config.getList( | ||
"otel.instrumentation.common.experimental.capture-http-headers.client.request"), |
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.
Shouldn't this part of ExperimentalConfig
?
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.
Hmm, you're right. Do you mind if I do that in a separate PR, after this one an #4255 are both merged? I'll just add another item to the TODO list in the PR description (and create a tracking issue for all of them once this PR is merged)
9b73886
to
d416ad6
Compare
private static final Cache<String, AttributeKey<List<String>>> requestKeysCache = | ||
Cache.newBuilder().setMaximumSize(32).build(); | ||
private static final Cache<String, AttributeKey<List<String>>> responseKeysCache = | ||
Cache.newBuilder().setMaximumSize(32).build(); | ||
|
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.
would it work to store the attribute name along with the header name in the configuration object, and avoid the need for this cache?
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.
We'd create different AttributeKey
values for same headers, if there were more than 1 instrumentations (that use them) loaded.
And I wanted to avoid exposing the attribute keys in any public APIs (although now that I think of it, it could be implemented without doing so)
...gent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientHttpAttributesExtractor.java
Outdated
Show resolved
Hide resolved
ffc9cf9
to
1e81792
Compare
f10e161
to
37b7c6c
Compare
This PR implements the HTTP request/response headers as span attributes semantic convention.
It includes:
HttpHeadersConfig
)Things that will be done in the followings PRs:
userAgent()
andhost()
implementations from all instrumentations, they're not needed anymoreHttpClientTest
andHttpServerTest
ServerInstrumenter
toHttpServerAttributesExtractor
HttpHeadersGetter
for all HTTP instrumentations? since we already have a method that returns values of any header in the extractor (it could implement update Get function in propagators to combine duplicate keys opentelemetry-specification#1884 properly too, for all HTTP instrumentations at the same time)