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

fix(ioredis): set net.peer.name attribute according to spec #241

Merged
merged 4 commits into from
Dec 14, 2020

Conversation

blumamir
Copy link
Member

@blumamir blumamir commented Nov 3, 2020

Which problem is this PR solving?

fixes #240

Short description of the changes

replace attribute net.peer.host with net.peer.name so it follows the specification Semantic conventions for database client calls

@blumamir blumamir requested a review from a team November 3, 2020 16:18
@codecov
Copy link

codecov bot commented Nov 3, 2020

Codecov Report

Merging #241 (f9e86ae) into master (12749ae) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #241   +/-   ##
=======================================
  Coverage   94.95%   94.95%           
=======================================
  Files         115      115           
  Lines        5990     5990           
  Branches      618      618           
=======================================
  Hits         5688     5688           
  Misses        302      302           

@naseemkullah
Copy link
Member

Should we parse the host to see if it is an IP and if so set NET_PEER_IP instead?

@blumamir
Copy link
Member Author

Should we parse the host to see if it is an IP and if so set NET_PEER_IP instead?

In my opinion it makes sense this way, because we populate the attribute from what the user configured, and not extracting IP address from the socket itself like the HTTP plugin is doing.
But I can see how it also makes sense to use the NET_PEER_IP.
If you think NET_PEER_IP is preferable I'll change it.

@naseemkullah
Copy link
Member

What I meant is that a host set by the user can either be a name or an IP. If we choose one PEER NAME is good, but ideally we check if the host is an IP and if so we set pEER IP but if it's a name we set PEER NAME

@naseemkullah
Copy link
Member

If you do choose to implement this check the following SO post might help. If not it's fine this lgtm since it's already an improvement
https://stackoverflow.com/questions/4460586/javascript-regular-expression-to-check-for-ip-addresses

@naseemkullah
Copy link
Member

In reality the spec should probably just have PEER HOST

@obecny obecny added the bug Something isn't working label Dec 14, 2020
@obecny obecny merged commit f87be0f into open-telemetry:master Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ioredis plugin publish attribute net.peer.host instead of net.peer.name
4 participants