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 network.interface.name attribute #1492

Merged
merged 5 commits into from
Oct 31, 2024

Conversation

ChrsMark
Copy link
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

@joaopgrassi
Copy link
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
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
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
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
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
Contributor

@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)

model/container/deprecated/registry-deprecated.yaml Outdated Show resolved Hide resolved
@lmolkova
Copy link
Contributor

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!)

@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
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
@lmolkova lmolkova enabled auto-merge (squash) October 31, 2024 01:31
@lmolkova lmolkova merged commit fa65d3a into open-telemetry:main Oct 31, 2024
13 of 14 checks passed
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