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

Add network metrics to process semantic conventions #2556

Merged
merged 14 commits into from
Jun 23, 2022
Merged

Add network metrics to process semantic conventions #2556

merged 14 commits into from
Jun 23, 2022

Conversation

codeboten
Copy link
Contributor

Changes

Adding network input/output as a process semantic convention. This could be used to report some of the metrics in the redis semconv pr

https://github.com/open-telemetry/opentelemetry-specification/pull/2547/files#r876330741

@codeboten codeboten marked this pull request as ready for review May 19, 2022 19:04
@codeboten codeboten requested review from a team May 19, 2022 19:04
@codeboten codeboten changed the title add network to process semantic conventions Add network metrics to process semantic conventions May 19, 2022
@carlosalberto
Copy link
Contributor

Probably I like in / out better, but that's just me ;) Overall LGTM.

@arminru arminru added area:semantic-conventions Related to semantic conventions spec:metrics Related to the specification/metrics directory labels May 20, 2022
Alex Boten added 2 commits May 25, 2022 13:00
add network to process semantic conventions
4c04321 
Adding network input/output as a process semantic convention.
add changelog, update input/output
dfe3421
@codeboten codeboten force-pushed the codeboten/add-network-bytes branch from 2669df4 to dfe3421 Compare May 25, 2022 20:01
@tigrannajaryan
Copy link
Member

I think this PR also violates our guidelines (along with other already existing metrics). See #2589
I am not blocking the PR since we may prefer to change the guidelines (I don't know).

@github-actions
Copy link

github-actions bot commented Jun 8, 2022

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jun 8, 2022
@tigrannajaryan
Copy link
Member

I think we can go ahead and merge this. #2589 needs to be resolved regardless and when it is solved all affected semantic conventions (including this one) may need to be updated.

@carlosalberto
Copy link
Contributor

Any concerns regarding the resulting labels (receive and transmit)? FWIW, they are also used in this PR: #2617 (it's almost always good to have uniformity)

If no concern arises, I will merge this PR.

@arminru
Copy link
Member

arminru commented Jun 22, 2022

@carlosalberto Here receive and transmit are added as attributes whereas in #2617 the direction is represented by two separate metrics with the direction embedded in their name. The consensus there seems to be going with this approach so I think we could also have this PR updated before merging. @codeboten would that work for you? This would spare you the follow-up PR right after merging the other one.

@codeboten
Copy link
Contributor Author

@arminru I was going to update whichever of this or #2617 was merged last with the changes to ensure consistency. I don't have a strong preference one way or the other. This PR already has enough approvals to be merged. I would suggest we merge this and I can update #2617 accordingly. WDYT?

@reyang
Copy link
Member

reyang commented Jun 23, 2022

@arminru I was going to update whichever of this or #2617 was merged last with the changes to ensure consistency. I don't have a strong preference one way or the other. This PR already has enough approvals to be merged. I would suggest we merge this and I can update #2617 accordingly. WDYT?

+1 on making the update in #2617.

@reyang reyang merged commit 5ee200d into open-telemetry:main Jun 23, 2022
beeme1mr pushed a commit to beeme1mr/opentelemetry-specification that referenced this pull request Aug 31, 2022
@beeme1mr
Add network metrics to process semantic conventions (open-telemetry#2556
1c35926 
)
beeme1mr pushed a commit to beeme1mr/opentelemetry-specification that referenced this pull request Aug 31, 2022
@beeme1mr
Add network metrics to process semantic conventions (open-telemetry#2556
4eb5100 
)
@ahayworth ahayworth mentioned this pull request Oct 7, 2022
Closed
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
Add network metrics to process semantic conventions (open-telemetry#2556
5f79ea3 
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reyang reyang reyang approved these changes

@arminru arminru arminru approved these changes

@carlosalberto carlosalberto carlosalberto approved these changes

@MrAlias MrAlias MrAlias approved these changes

@cijothomas cijothomas cijothomas approved these changes

@jmacd jmacd jmacd approved these changes

@bogdandrutu bogdandrutu bogdandrutu approved these changes

Assignees

@arminru arminru

Labels
area:semantic-conventions Related to semantic conventions spec:metrics Related to the specification/metrics directory
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@codeboten @carlosalberto @tigrannajaryan @arminru @reyang @bogdandrutu @jmacd @cijothomas @MrAlias