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

[metricbeat]update apiserver metrics #12922

Merged

Conversation

odacremolbap
Copy link
Contributor

@odacremolbap odacremolbap commented Jul 16, 2019

As stated here, updating metrics and adding new ones

  • Added common metrics to other kubernetes components related to process and HTTP.
  • Added etcd, audit, and apiserver request metrics
  • A hack (something to improve in the future imho) to select apiserver_request_total if it is informed, apiserver_request_count if not, using the same event path request.count
  • Both apiserver_request_duration_seconds and apiserver_request_latencies will be retrieved, although only the first should be used, the later will allow current dashboard to work.

@odacremolbap odacremolbap requested review from a team as code owners July 16, 2019 07:45
@odacremolbap odacremolbap self-assigned this Jul 16, 2019
@odacremolbap odacremolbap added Metricbeat Metricbeat containers Related to containers use case in progress Pull request is currently in progress. Team:Integrations Label for the Integrations team labels Jul 16, 2019
@odacremolbap odacremolbap added review and removed in progress Pull request is currently in progress. labels Jul 16, 2019
"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"),
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

@exekias exekias left a 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 {
Copy link
Contributor

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

Copy link
Contributor Author

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

@odacremolbap odacremolbap added in progress Pull request is currently in progress. and removed review labels Jul 17, 2019
@odacremolbap odacremolbap added review and removed in progress Pull request is currently in progress. labels Jul 17, 2019
@exekias
Copy link
Contributor

exekias commented Jul 18, 2019

thank you for the extra effort to maintain bwc!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
containers Related to containers use case Metricbeat Metricbeat review Team:Integrations Label for the Integrations team v7.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants