Skip to content

Add network.interface.name attribute#1492

Merged
lmolkova merged 5 commits intoopen-telemetry:mainfrom
ChrsMark:add_network_interface
Oct 31, 2024
Merged

Add network.interface.name attribute#1492
lmolkova merged 5 commits intoopen-telemetry:mainfrom
ChrsMark:add_network_interface

Conversation

@ChrsMark
Copy link
Copy Markdown
Member

@ChrsMark ChrsMark commented Oct 18, 2024

Changes

ref: #308 (comment)

As discussed in the above referenced issue, this PR introduces the network.interface.name attribute and replace with it the system.device attribute of system and container network metrics.

Note: I'm not sure if this change can be reflected in schema-next.yaml 🤔 .

Merge requirement checklist

@ChrsMark ChrsMark requested a review from a team as a code owner October 18, 2024 07:57
@ChrsMark ChrsMark requested a review from a team October 18, 2024 07:57
@ChrsMark ChrsMark requested review from a team as code owners October 18, 2024 07:57
@ChrsMark ChrsMark force-pushed the add_network_interface branch from ac2123c to 641844d Compare October 18, 2024 07:58
@joaopgrassi
Copy link
Copy Markdown
Member

Note: I'm not sure if this change can be reflected in schema-next.yaml 🤔 .

It can, you can list the metrics that are affected and the attribute change. See for example: https://github.com/open-telemetry/semantic-conventions/blob/main/schemas/1.28.0#L48

Copy link
Copy Markdown
Member

@joaopgrassi joaopgrassi left a comment

Choose a reason for hiding this comment

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

@ChrsMark ChrsMark requested a review from a team October 21, 2024 08:23
@ChrsMark
Copy link
Copy Markdown
Member Author

Thank's @joaopgrassi . My concern here is that system.device is not entirely removed, it's still used as an attribute in other metrics. Here we just replace its usage in specific metrics. I guess this can be reflected in the schema-next as you mentioned, but should this also be added in the deprecated file(s)?

@joaopgrassi
Copy link
Copy Markdown
Member

joaopgrassi commented Oct 21, 2024

but should this also be added in the deprecated file(s)?

I believe yes, each "area" now has its own deprecated file, so within that usage, the attribute is considered deprecated. There then it's also mentioned which one to use instead.

@lmolkova @jsuereth is this correct in your view? Not sure if we had such case before 🤔

@ChrsMark ChrsMark force-pushed the add_network_interface branch from 06ed333 to 97552c3 Compare October 21, 2024 15:17
@ChrsMark
Copy link
Copy Markdown
Member Author

ChrsMark commented Oct 21, 2024

but should this also be added in the deprecated file(s)?

I believe yes, each "area" now has its own deprecated file, so within that usage, the attribute is considered deprecated. There then it's also mentioned which one to use instead.

@lmolkova @jsuereth is this correct in your view? Not sure if we had such case before 🤔

Appears that we can't model this right now?

I get:

> make fix

Diagnostic report:

  × The attribute id `system.device` is declared multiple times in the
  │ following groups:
  │ ["registry.container.metrics.deprecated",
  │ "registry.system.metrics.deprecated", "registry.system"]

/cc @joaopgrassi

Copy link
Copy Markdown
Member

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

LGTM (with some minor suggestions to make CI happy)

Comment thread model/container/deprecated/registry-deprecated.yaml Outdated
@lmolkova
Copy link
Copy Markdown
Member

lmolkova commented Oct 24, 2024

Appears that we can't model this right now?

There is no need to deprecate attributes if it's still used.

The only thing you need to do is to write the proper section in the schema-next (which you already did!)

Comment thread model/system/deprecated/registry-deprecated.yaml Outdated
Comment thread schema-next.yaml
@ChrsMark ChrsMark force-pushed the add_network_interface branch 2 times, most recently from 3bf1711 to ff3f5df Compare October 25, 2024 07:49
@ChrsMark
Copy link
Copy Markdown
Member Author

@open-telemetry/semconv-system-approvers could you review this one please ?

@ChrsMark ChrsMark force-pushed the add_network_interface branch from 190d9a2 to 14ee552 Compare October 30, 2024 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

7 participants