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

[chore] Move system metric attributes to the registry #867

Merged
merged 8 commits into from
Apr 17, 2024

Conversation

gregkalapos
Copy link
Member

@gregkalapos gregkalapos commented Apr 2, 2024

Changes

Moves system.* attributes to the registry from system metrics.

Merge requirement checklist

@gregkalapos gregkalapos marked this pull request as ready for review April 2, 2024 19:36
@gregkalapos gregkalapos requested review from a team April 2, 2024 19:36
Add sub-sections
Add <!-- toc -->
Add deprecated attribtutes to the list.
@gregkalapos gregkalapos force-pushed the MoveSystemAttributes branch from 7db66a0 to 893c4e6 Compare April 10, 2024 13:22
@gregkalapos gregkalapos changed the title Move system metric attributes to the registry [chore] Move system metric attributes to the registry Apr 10, 2024
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

Not sure if the state of this PR is "ready for review" already but my comment seems to be still unresolved?

I think we have concluded on having the registry in a signal generic form. In this I left some suggestions to remove any mention to "Metrics" from the registry's attributes.

And we will still need to override the system.cpu.state within the system metrics spec.

Let me know @gregkalapos if I miss anything or there is anything that is unclear ;).

model/registry/system.yaml Outdated Show resolved Hide resolved
model/registry/system.yaml Outdated Show resolved Hide resolved
model/registry/system.yaml Outdated Show resolved Hide resolved
model/registry/system.yaml Outdated Show resolved Hide resolved
model/registry/system.yaml Outdated Show resolved Hide resolved
model/registry/system.yaml Outdated Show resolved Hide resolved
model/registry/system.yaml Outdated Show resolved Hide resolved
model/registry/system.yaml Outdated Show resolved Hide resolved
@gregkalapos
Copy link
Member Author

Not sure if the state of this PR is "ready for review" already but my #867 (comment) seems to be still unresolved?

Sorry for the confusion - let's discussed it in that thread and depending on what we agree on, I'll move on with the suggestions.

@gregkalapos gregkalapos requested a review from ChrsMark April 11, 2024 09:45
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

Thank's @gregkalapos! Looks good from my side.

@joaopgrassi joaopgrassi merged commit 4728f63 into open-telemetry:main Apr 17, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants