-
Notifications
You must be signed in to change notification settings - Fork 272
[k8s] Add k8s node allocatable metrics #2267
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
[k8s] Add k8s node allocatable metrics #2267
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct that there are no attributes in any of these metrics?
746d981
to
15981b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned at https://github.com/open-telemetry/semantic-conventions/pull/2267/files#r2120289448, instead of just defining the metrics as-is from the Collector's implementation we should work on defining them correctly.
We do this for all the metrics of #1032 and we have already "breaking" changes that we plan to handle through a specific migration process once we have k8s metrics to GA. We document these diffs at https://github.com/open-telemetry/semantic-conventions/blob/main/docs/non-normative/k8s-migration.md
That said, the missing piece here is if we should have a k8s.node.allocatable.*
namespace with all the sub-resources attached (cpu, memory etc) or if we should re-use the existing k8s.node.cpu.*
, k8s.node.memory
namespaces. I don't have a strong preference for any of these 2, but maybe picking the k8s.node.allocatable.*
namespace will better align context-wise?
15981b0
to
95867b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also list how these metrics are different to what Collect has today? We track those diffs at https://github.com/open-telemetry/semantic-conventions/blob/main/docs/non-normative/k8s-migration.md#summary-of-changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank's for contributing this!
@open-telemetry/specs-semconv-maintainers @open-telemetry/specs-semconv-approvers this one has enough approvals from the K8s SIG. Should be on your side now :) |
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
6aa4e1b
to
c0bd712
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR is not affected by the discussion on naming UpDownCounters https://github.com/open-telemetry/semantic-conventions/pull/2317/files, so giving my approval.
For the implementation, consider this important fact when using UpDownCounters: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/general/metrics.md#consistent-updowncounter-timeseries
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Fixes #2243
Changes
added
follow-up issue to handle the changes in k8scluster receiver here
Merge requirement checklist