-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[kubernetes] Add last_terminated_reason_timestamp metric #39200
[kubernetes] Add last_terminated_reason_timestamp metric #39200
Conversation
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
Tania minor notes:
Do you think shall we also add: |
Co-authored-by: Andrew Gizas <andreas.gkizas@elastic.co>
"kube_pod_container_status_terminated_reason": p.LabelMetric("status.reason", "reason"), | ||
"kube_pod_container_status_waiting_reason": p.LabelMetric("status.reason", "reason"), | ||
"kube_pod_container_status_last_terminated_reason": p.LabelMetric("status.last_terminated_reason", "reason"), | ||
"kube_pod_container_status_last_terminated_timestamp": p.Metric("status.last_terminated_reason.sec"), |
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.
Here you map kube_pod_container_status_last_terminated_timestamp
ksm metric to elasticsearch field kubernetes.container.status.last_terminated_reason.sec
. But then , the field you declare in the fields.yaml is last_terminated_reason_timestamp
. Am I missing something?
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.
my mistake, reverted this - 180afdf
It is a leftover, I was considering other naming, not really sure regarding the name last_terminated_reason_timestamp
. For example in state_cronjob - https://github.com/elastic/beats/blob/main/metricbeat/module/kubernetes/state_cronjob/state_cronjob.go#L33-L34 is used *.sec
for time metrics, but then I figured out that there is not really a pattern for time metrics, for example:
job - time.*
(type of the field: data)
statefulset - no time
or sec
in name (type: long)
storageclass - no time
or sec
in name (type: date)
namespace - created.sec
, type: double
do you have any thoughts on naming?
maybe use last_terminated
(similar to *.created
with the type: double) ?
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.
We already have the field status.last_terminated_reason
that corresponds to kube_pod_container_status_last_terminated_reason
.
If we follow the same convention for kube_pod_container_status_last_terminated_timestamp
let's name the field status.last_terminated_timestamp
. There is no need to add the reason
in the field name as it is just the timestamp of the last termination. It is not related with the reason.
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.
done in 90d82b6
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
@gizas I've added steps to test and the screenshot
can you provide the use case? we already provide the |
I was thinking that the code is a number and probably for those who use APIs is better to check for specific code and if is present then to return it. But not so important for now as not specifically asked. Skip it |
Proposed commit message
Please explain:
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Author's Checklist
How to test this PR locally
kind create cluster
elastic-package-0.98.2 stack up -d -v --version=8.14.0-SNAPSHOT
mode="run"
and run:Related issues
Use cases
Screenshots
Logs