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.

Sign up for GitHub

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

Jump to bottom

Add new fields, delete deprecated ones and update dashboards for apiserver and controllermanager #3825

Merged
merged 6 commits into from
Jul 29, 2022
Merged

Add new fields, delete deprecated ones and update dashboards for apiserver and controllermanager #3825

merged 6 commits into from
Jul 29, 2022

Conversation

MichaelKatsoulis
Copy link
Contributor

@MichaelKatsoulis MichaelKatsoulis commented Jul 25, 2022 •
edited
Loading

What does this PR do?

Follow up PR to elastic/beats#31834 and elastic/beats#32007 in which kubernetes apiserver and controllermanager metricsets where updated to get rid off deprecated fields and collect new ones.

This PR updates the integrations data streams as well and the OOTB dashboards to match those created in the modules.

Also,
All OOTB dashboards are updated to include API server dashboard in the dashboards list.
controllermanager, scheduler and proxy are left out of the list as in some Environments (cloud) they don't have data.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

How to test this PR locally

Use elastic-package to build up the stack and install kubernetes integration with apiserver and controllermanager enabled. Check the metrics collected and the OOTB dashboards that all visualisations show data.

Screenshots

Dashboards list

Screenshot 2022-07-27 at 12 43 19 PM

Apiserver

apiserver1

apiserver2

Controllermanager

controllermanager1

controllermanager2

@MichaelKatsoulis MichaelKatsoulis added the enhancement New feature or request label Jul 25, 2022
@MichaelKatsoulis MichaelKatsoulis requested review from a team as code owners July 25, 2022 12:52
@elasticmachine
Copy link

elasticmachine commented Jul 25, 2022 •
edited
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-07-29T07:46:17.583+0000

  • Duration: 32 min 40 sec

Test stats 🧪

Test Results
Failed 0
Passed 90
Skipped 0
Total 90

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented Jul 25, 2022 •
edited
Loading

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (0/0) 💚
Files 100.0% (0/0) 💚 3.046
Classes 100.0% (0/0) 💚 3.046
Methods 94.872% (74/78) 👍 5.501
Lines 100.0% (0/0) 💚 9.908
Conditionals 100.0% (0/0) 💚

tetianakravchenko
tetianakravchenko reviewed Jul 26, 2022
View reviewed changes
Copy link
Contributor

@tetianakravchenko tetianakravchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update sample_event.json of apiserver and controllermanager with new fields

@MichaelKatsoulis MichaelKatsoulis marked this pull request as draft July 26, 2022 09:43
@MichaelKatsoulis
Copy link
Contributor Author

please update sample_event.json of apiserver and controllermanager with new fields

@tetianakravchenko due to this PR I found a problem with how some fields are stored for controllermanager metricset. see elastic/beats#32486.
I will update the integration accordingly. I will mark it as draft for now.

@MichaelKatsoulis MichaelKatsoulis self-assigned this Jul 26, 2022
@MichaelKatsoulis MichaelKatsoulis marked this pull request as ready for review July 27, 2022 10:26
@MichaelKatsoulis
Copy link
Contributor Author

@tetianakravchenko I updated the PR following the merge of elastic/beats#32486.
Ci fails because it seems that the elastic agent that elastic package uses to run the tests, is still not updated with the latest metricbeat.
Locally the tests pass.

tetianakravchenko
tetianakravchenko approved these changes Jul 27, 2022
View reviewed changes
Copy link
Contributor

@tetianakravchenko tetianakravchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I've left a minor question

packages/kubernetes/data_stream/controllermanager/fields/fields.yml
metric_type: counter
description: Response count
- name: request.count
description: Request duration, sum in microseconds
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should here be added units: micros ?
https://github.com/elastic/observability-dev/blob/main/docs/dc/integrations/development/generic_guidelines.md#specify-metric-types-and-units

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8141f01

@drewdaemon
Copy link
Contributor

Tried to review but ran into elastic/elastic-package#914. Waiting for resolution. Please reach out directly if you know what might be going on.

@MichaelKatsoulis
Copy link
Contributor Author

/test

2 similar comments
@MichaelKatsoulis
Copy link
Contributor Author

/test

@MichaelKatsoulis
Copy link
Contributor Author

/test

@tetianakravchenko tetianakravchenko self-assigned this Jul 28, 2022
drewdaemon
drewdaemon approved these changes Jul 28, 2022
View reviewed changes
Copy link
Contributor

@drewdaemon drewdaemon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure—did you mean to leave the Controller Manager dashboard out of the list here?

Screen Shot 2022-07-28 at 11 38 43 AM

Thank you for using Lens and "by-value" panels for the new API Server dashboard.

Building dashboards on a snapshot version is risky and should be avoided when possible. This is because Kibana dashboard and visualization functionality can change any time before the GA release, breaking your dashboards (see best practices).

That said, I think in your case you'll be okay since

  1. we're past the 8.4 Kibana feature freeze
  2. most of these are just changes to the markdown

@tetianakravchenko
Copy link
Contributor

/test

@tetianakravchenko
Copy link
Contributor

Just to make sure—did you mean to leave the Controller Manager dashboard out of the list here?

there are as well some other dashboards that are left out, like Kubernetes Scheduler metrics, and Kubernetes Proxy metrics. Now I am not sure why Kubernetes API Server was added here, along with the k8s resources dashboards.

I am going to merge this pr, as the question above does not seem to be a blocker here and can be addressed later.

@tetianakravchenko tetianakravchenko merged commit 87a87e5 into elastic:main Jul 29, 2022
@MichaelKatsoulis MichaelKatsoulis mentioned this pull request Nov 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tetianakravchenko tetianakravchenko tetianakravchenko approved these changes

@drewdaemon drewdaemon drewdaemon approved these changes

@ChrsMark ChrsMark Awaiting requested review from ChrsMark

Assignees

@MichaelKatsoulis MichaelKatsoulis

@tetianakravchenko tetianakravchenko

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MichaelKatsoulis @elasticmachine @drewdaemon @tetianakravchenko