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.

Sign up for GitHub

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

Jump to bottom

Split NetAttributesExtractor into NetClientAttributesExtractor and NetServerAttributesExtractor #4287

Merged
merged 32 commits into from
Oct 7, 2021
Merged

Split NetAttributesExtractor into NetClientAttributesExtractor and NetServerAttributesExtractor #4287

merged 32 commits into from
Oct 7, 2021

Conversation

trask
Copy link
Member

@trask trask commented Oct 4, 2021 •
edited
Loading

Alternative to #4278 that implements parallel hierarchies:

  • NetClientAttributesExtractor
  • InetSocketAddressNetClientAttributesExtractor

and

  • NetClientAttributesExtractor
  • InetSocketAddressNetClientAttributesExtractor

Note: peer.service only applies to client spans (afaict), so PeerServiceAttributesExtractor is not split, and only applies to NetClientAttributesExtractor.

@trask trask mentioned this pull request Oct 4, 2021
Closed
@mateuszrzeszutek
Copy link
Member

Should we go ahead with the NetAttributesServerExtractor/NetAttributesClientExtractor idea? It matches the recently changes flavor() methods in HTTP extractors.

@trask
Copy link
Member Author

trask commented Oct 5, 2021

Should we go ahead with the NetAttributesServerExtractor/NetAttributesClientExtractor idea? It matches the recently changes flavor() methods in HTTP extractors.

This PR is updated now with this approach 👍

mateuszrzeszutek
mateuszrzeszutek reviewed Oct 6, 2021
View reviewed changes
...etry/instrumentation/api/instrumenter/net/InetSocketAddressNetAttributesServerExtractor.java Outdated Show resolved Hide resolved
...etry/instrumentation/api/instrumenter/net/InetSocketAddressNetAttributesClientExtractor.java Outdated Show resolved Hide resolved
...etry/instrumentation/api/instrumenter/net/InetSocketAddressNetAttributesClientExtractor.java Outdated Show resolved Hide resolved
.../instrumentation/api/instrumenter/net/InetSocketAddressNetAttributesClientExtractorTest.java Outdated Show resolved Hide resolved
.../instrumentation/api/instrumenter/net/InetSocketAddressNetAttributesServerExtractorTest.java Outdated Show resolved Hide resolved
.../java/io/opentelemetry/javaagent/instrumentation/jedis/v1_4/JedisNetAttributesExtractor.java Outdated Show resolved Hide resolved
@trask
Copy link
Member Author

trask commented Oct 6, 2021

please see this commit for attempt to expose peerService to library instrumentation based on discussion in #4235

@trask
Copy link
Member Author

trask commented Oct 6, 2021

please see this commit for attempt to expose peerService to library instrumentation based on discussion in #4235

I also added similar for Dubbo

@trask
Copy link
Member Author

trask commented Oct 6, 2021

please see this commit for attempt to expose peerService to library instrumentation based on discussion in #4235

I also added similar for Dubbo

and gRPC

@trask trask changed the title NetAttributesExtractor optimization Split NetAttributesExtractor into NetClientAttributesExtractor and NetServerAttributesExtractor Oct 6, 2021
@trask
Copy link
Member Author

trask commented Oct 7, 2021

Going ahead and merging since it's large and conflict-y. Happy to address any additional thoughts in follow-ups.

@trask trask merged commit 63a2383 into open-telemetry:main Oct 7, 2021
@trask trask deleted the net-extractors branch October 7, 2021 22:40
@laurit laurit mentioned this pull request Oct 14, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mateuszrzeszutek mateuszrzeszutek mateuszrzeszutek approved these changes

@anuraaga anuraaga Awaiting requested review from anuraaga

@iNikem iNikem Awaiting requested review from iNikem

@jkwatson jkwatson Awaiting requested review from jkwatson

@laurit laurit Awaiting requested review from laurit

@pavolloffay pavolloffay Awaiting requested review from pavolloffay

@tylerbenson tylerbenson Awaiting requested review from tylerbenson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@trask @mateuszrzeszutek