-
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
[metricbeat]update apiserver metrics #12922
[metricbeat]update apiserver metrics #12922
Conversation
"apiserver_request_duration_seconds": prometheus.Metric("request.duration.us", prometheus.OpMultiplyBuckets(1000000)), | ||
"apiserver_request_latencies": prometheus.Metric("request.latency"), | ||
"apiserver_request_total": prometheus.Metric("request.count"), | ||
"apiserver_request_count": prometheus.Metric("request.beforev14.count"), |
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 there any version where both apiserver_request_total
and apiserver_request_count
cohexists? If the answer is no you could just use "request.count"
in both cases
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.
unfortunately both exists at the same metric resource.
There are way better way dealing with this than this one with some revamping of the prometheus package, but that is far out of scope of this PR
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.
Thanks for the explanation, one more question. Are the values/types under these different?
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.
same everything but for an added label at the non deprecated apiserver_request_total
for dry runs
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.
Mostly looking good, I left a question.
Could you please update the PR description to add more detail about the change? It's great to link other issues & PRs but description should be self explanatory
// to metrics retrieval in general, so mappings, retrieved metrics, ... can be | ||
// modified on events. Current design is limiting | ||
if ok, _ := event.HasKey("request.beforev14.count"); ok { | ||
if ok, _ := event.HasKey("request.count"); !ok { |
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.
Not sure if I understood this right, but if the old count metric has different labels than the new one.. i guess they should never end up in the same document? that would mean this condition is always satisfied
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.
so true, thanks for spotting that
thank you for the extra effort to maintain bwc! |
As stated here, updating metrics and adding new ones
apiserver_request_total
if it is informed,apiserver_request_count
if not, using the same event pathrequest.count
apiserver_request_duration_seconds
andapiserver_request_latencies
will be retrieved, although only the first should be used, the later will allow current dashboard to work.