-
Notifications
You must be signed in to change notification settings - Fork 189
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 k8s.container.status.state metric #1784
base: main
Are you sure you want to change the base?
Conversation
8de6ad2
to
726203e
Compare
brief: "" | ||
instrument: updowncounter | ||
unit: "" | ||
attributes: |
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 we need to name these as k8s.container.status.state
and k8s.container.status.reason
so as to have meaningful names in case we want to re-use them in other signals like Entities (k8s.container.reason
does not make a lot of sense).
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.
Renamed the attributes, thanks!
model/k8s/metrics.yaml
Outdated
- ref: k8s.container.state | ||
requirement_level: required | ||
- ref: k8s.container.reason | ||
requirement_level: required |
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.
If we have it as required
, the problem is that states which do not have a reason won't have it provided. Maybe that could be left "unknown" or just empty?
@open-telemetry/semconv-k8s-approvers @open-telemetry/specs-semconv-approvers thoughts?
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.
Removed the "required" for now, but would appreciate suggestions what we want to do here
726203e
to
6923556
Compare
Fixes #1672
Changes
Adds
k8s.container.status.state
metric, it would allow us to alert and monitor containers in not ready state.I'm still not sure if this should be multiple different metrics or a single one 🤔
The current problems, with single metric:
Waiting state - "ContainerCreating", "CrashLoopBackOff", "CreateContainerConfigError", "ErrImagePull", "ImagePullBackOff"
Terminated state - "OOMKilled", "Completed", "Error", "ContainerCannotRun"
Alternative approach would be to do what KSM does:
k8s.container.status.state
metric withoutreason
attribute.k8s.container.status.waiting_reason
metric for waiting reason enum.k8s.container.status.terminated_reason
metric for terminated reason enum.This is not intended to merge, I would appreciate any feedback to see what we want to do here.
Merge requirement checklist
[chore]