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

add definition for state attribute of system.network.connections #2663

Merged
merged 4 commits into from
Jul 18, 2022

Conversation

codeboten
Copy link
Contributor

@codeboten codeboten commented Jul 14, 2022

Fixes #2623

Changes

Replacing wikipedia link with set values for the state. I would add that the states listed here only apply to TCP protocol, which reinforces my thinking that the system.network.connections metric should be split by protocol. Asking reviewers to consider this.

If folks agree, I will open a follow up PR to mark system.network.connections as deprecated in favour of:

  • system.network.tcp.connections
  • system.network.udp.connections

Replacing wikipedia link with set values for the state. I would add that the states listed here only apply to TCP protocol, which reinforces my thinking that the system.network.connections metric should be split by protocol. Asking reviewers to consider this. If folks agree, I will open a follow up PR to mark `system.network.connections` as deprecated in favour of `system.network.tcp.connections` and `system.network.udp.connections`.

Fixes open-telemetry#2623
@codeboten codeboten requested review from a team as code owners July 14, 2022 17:49
@arminru arminru added area:semantic-conventions Related to semantic conventions spec:metrics Related to the specification/metrics directory labels Jul 18, 2022
Alex Boten and others added 2 commits July 18, 2022 10:00
@arminru arminru merged commit 1fd2db2 into open-telemetry:main Jul 18, 2022
@arminru
Copy link
Member

arminru commented Jul 18, 2022

I would add that the states listed here only apply to TCP protocol, which reinforces my thinking that the system.network.connections metric should be split by protocol. Asking reviewers to consider this.
If folks agree, I will open a follow up PR to mark system.network.connections as deprecated in favour of:

The semantics introduced by this PR allowing to distinguish TCP and UDP by the presence or absence of state seem to be fine with me but splitting it up might be more reasonable, yes.

@codeboten codeboten deleted the codeboten/fix-2623 branch July 18, 2022 18:02
codeboten pushed a commit to codeboten/opentelemetry-specification that referenced this pull request Jul 18, 2022
As a follow up to open-telemetry#2663, since UDP has no state attribute, I propose UDP and TCP connection metrics should be split into protocol specific metric for each supported protocol.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the definition of state
4 participants