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

Capture net.host.name for netty #6892

Merged
merged 15 commits into from
Oct 19, 2022
Merged

Capture net.host.name for netty #6892

merged 15 commits into from
Oct 19, 2022

Conversation

trask
Copy link
Member

@trask trask commented Oct 17, 2022

This may be a regression in 1.19.0 because you can no longer reconstruct the original url for netty server spans (previously http.host was captured which could be used).

@trask trask changed the title Netty Capture net.host.name for netty Oct 17, 2022
@trask trask marked this pull request as ready for review October 18, 2022 02:20
@trask trask requested a review from a team as a code owner October 18, 2022 02:20
Copy link
Member

@mateuszrzeszutek mateuszrzeszutek left a comment

Choose a reason for hiding this comment

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

WDYT about introducing a separate, custom AttributesExtractor implementation that extracts that just for netty?

I actually planned to fix this issue, and implement the HTTP net.host.*/net.peer.* attributes correctly as one of the next steps - I imagined that we'd change the HttpServerAttributesExtractor so that it requires both HttpServerAttributesGetter and a NetServerAttributesGetter. This way you wouldn't be using the NetServerAttributesExtractor in HTTP instrumentations, but just the HttpServerAttributesExtractor - and it'd set all the net attributes, with all that additional HTTP logic applied (e.g. parsing the Host header, not setting 80/443 ports, etc)

Comment on lines 123 to 127
@CanIgnoreReturnValue
public HttpServerTestOptions setHasNetAttributes(boolean hasNetAttributes) {
this.hasNetAttributes = hasNetAttributes;
return this;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we leave a TODO refactor to use httpAttributes here? I think it's fine to do it like this for now, but long term we'll probably want to use the same mechanism for all http/net attributes

Copy link
Member

@mateuszrzeszutek mateuszrzeszutek left a comment

Choose a reason for hiding this comment

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

Looks great! 🚀

@trask trask merged commit f488d94 into open-telemetry:main Oct 19, 2022
@trask trask deleted the netty branch October 19, 2022 16:21
github-actions bot pushed a commit that referenced this pull request Oct 19, 2022
This may be a regression in 1.19.0 because you can no longer reconstruct
the original url for netty server spans (previously `http.host` was
captured which could be used).
trask added a commit that referenced this pull request Oct 19, 2022
Clean cherry-pick of #6892 to the `release/v1.19.x` branch.

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
dmarkwat pushed a commit to dmarkwat/opentelemetry-java-instrumentation that referenced this pull request Oct 22, 2022
This may be a regression in 1.19.0 because you can no longer reconstruct
the original url for netty server spans (previously `http.host` was
captured which could be used).
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 23, 2022
This may be a regression in 1.19.0 because you can no longer reconstruct
the original url for netty server spans (previously `http.host` was
captured which could be used).
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 31, 2022
This may be a regression in 1.19.0 because you can no longer reconstruct
the original url for netty server spans (previously `http.host` was
captured which could be used).
trask added a commit that referenced this pull request Nov 1, 2022
Methods were removed in #6892, which was backported to the 1.19.x branch
and included in the 1.19.1 release.
github-actions bot pushed a commit that referenced this pull request Nov 1, 2022
Methods were removed in #6892, which was backported to the 1.19.x branch
and included in the 1.19.1 release.
trask pushed a commit that referenced this pull request Nov 30, 2022
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Dec 4, 2022
This may be a regression in 1.19.0 because you can no longer reconstruct
the original url for netty server spans (previously `http.host` was
captured which could be used).
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.

None yet

3 participants