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

move common hardware attributes and metrics to registry #1030

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

trisch-me
Copy link
Contributor

@trisch-me trisch-me commented May 10, 2024

This is the first PR out of many for hardware metrics.

The first PR is about generic attributes used by all hardware metrics. I decided to split it into multiple PRs to make review easier because it's easy to miss something with a lot of changed files (I have started doing it with 5 metrics and lost myself track on things :) ).

I delete portions of docs/system/hardware-metrics.md file so that reviewers could see what has been changed now. This file will be deleted later as soon as it will be ported completely.

Hardware wasn't moved to the yaml schema, so I'm doing here both introducing it to the registry and creating appropriate yaml files for each hardware metric

I need this PR merged before I will create other PRs (half of them is ready but based on this PR)

Merge requirement checklist

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.

Please also add a changelog entry.

model/metrics/hardware/common.yaml Outdated Show resolved Hide resolved
unit: "{errors}"
extends: metric.hardware.attributes
attributes:
- ref: hw.error.type
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- ref: hw.error.type
- ref: error.type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have answered in another similar comment

Copy link
Contributor

@lmolkova lmolkova May 24, 2024

Choose a reason for hiding this comment

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

Since there was no defined attribute for hw.error.type, my strong preference would be not to introduce one and instead use generic error.type.

Otherwise, we'd end up with another attribute that should be immediately deprecated.

model/metrics/hardware/common.yaml Outdated Show resolved Hide resolved
model/metrics/hardware/common.yaml Show resolved Hide resolved
model/metrics/hardware/common.yaml Outdated Show resolved Hide resolved
value: 'temperature'
brief: 'Temperature'
stability: experimental
- id: voltage
Copy link
Contributor

Choose a reason for hiding this comment

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

same here - voltage seems confusing as a type

Copy link
Contributor

Choose a reason for hiding this comment

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

so the suggestion is to use voltage_sensor

Copy link
Contributor

Choose a reason for hiding this comment

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

@lmolkova See my previous comment for temperature.

docs/system/hardware-metrics.md Show resolved Hide resolved
docs/system/hardware-metrics.md Show resolved Hide resolved
docs/hardware/common.md Outdated Show resolved Hide resolved
docs/hardware/common.md Show resolved Hide resolved
@lmolkova lmolkova changed the title [chore] add common hardware attributes add common hardware attributes and metrics May 10, 2024
@trask
Copy link
Member

trask commented May 10, 2024

cc @bertysentry

@trisch-me
Copy link
Contributor Author

@lmolkova I'm confused by your comments. I haven't created anything new. I have extracted hardware from one md file into yaml. As I stated in description I will do this in batches because there will be open questions and to make review easier, so I have extracted only first portion

Co-authored-by: Liudmila Molkova <limolkova@microsoft.com>
@trisch-me trisch-me added the Skip Changelog Label to skip the changelog check label May 11, 2024
@trisch-me trisch-me changed the title add common hardware attributes and metrics move common hardware attributes and metrics to registry May 11, 2024
@bertysentry
Copy link
Contributor

bertysentry commented May 13, 2024

@trisch-me Thank you so much for taking the time to move the hardware semconv to YAML! (I was supposed to do it but couldn't find the time... and I was afraid the tooling wouldn't work well on some of the specificities of the hardware metrics, with common attributes, etc.).

Let me know if you need any help with that!

If you want to see these metrics in real life (to get a better understanding of their purpose and use), you can check these Grafana dashboards:

Thanks!

@trisch-me
Copy link
Contributor Author

thanks @bertysentry for your comments. If you don't mind I will add you as reviewer to all my PRs related to the moving hw metrics to the yaml files/registry

off-topic - I think we have met in the OTel booth during this Kubecon conference in Paris :)

@bertysentry
Copy link
Contributor

@trisch-me I confirm we met in Paris! It's funny when you realize usernames in GitHub are actual people! 😅

I'll be happy to review any PR on this topic!

@trisch-me
Copy link
Contributor Author

trisch-me commented May 21, 2024

blocked by #1049
PR is merged

@@ -44,6 +44,7 @@ body:
- area:gcp
- area:gen-ai
- area:graphql
- area:hardware
Copy link
Contributor

Choose a reason for hiding this comment

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

could we be consistent with the root namespace?

Suggested change
- area:hardware
- area:hw

(and in other places)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hw as abbreviation seems strange to me in registry so I prefer hardware, but I haven't changed the namespace.
@bertysentry do you think we should/can change common namespace for hardware to be indeed hardware and not hw?

@lmolkova
Copy link
Contributor

@lmolkova I'm confused by your comments. I haven't created anything new. I have extracted hardware from one md file into yaml. As I stated in description I will do this in batches because there will be open questions and to make review easier, so I have extracted only first portion

I created #1071 to keep track of HW naming issues. I still suggest to do a bare minimum changes to existing HW metrics so that we don't need to deprecated a bunch of things later (hw.error.type, and hw.type values).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog Label to skip the changelog check
Projects
Status: In Discussions
Development

Successfully merging this pull request may close these issues.

None yet

5 participants