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

[System] Replace TSVB gauges in system metrics and host overview dashboards #4975

Closed
wants to merge 5 commits into from

Conversation

drewdaemon
Copy link
Contributor

@drewdaemon drewdaemon commented Jan 11, 2023

What does this PR do?

System and host metric overview dashboards now use Lens metrics instead of TSVB gauges.

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.

Related issues

Screenshots

Host overview

Before

Screen Shot 2023-01-11 at 5 27 42 PM

After

Screen Shot 2023-01-11 at 5 28 21 PM

System overview

Before

Screen Shot 2023-01-11 at 5 28 00 PM

After

Screen Shot 2023-01-11 at 5 27 22 PM

@drewdaemon drewdaemon added the enhancement New feature or request label Jan 11, 2023
@drewdaemon drewdaemon changed the title use lens metric instead of TSVB gauge Enhancements for system metrics and host overview dashboards Jan 11, 2023
@drewdaemon drewdaemon changed the title Enhancements for system metrics and host overview dashboards Replace TSVB gauges in system metrics and host overview dashboards Jan 11, 2023
@elasticmachine
Copy link

elasticmachine commented Jan 12, 2023

💔 Build Failed

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: 2023-06-27T19:34:48.862+0000

  • Duration: 14 min 27 sec

Steps errors 3

Expand to view the steps failures

Test integration: system
  • Took 1 min 42 sec . View more details here
  • Description: eval "$(../../build/elastic-package stack shellinit)" ../../build/elastic-package test -v --report-format xUnit --report-output file --test-coverage
Google Storage Download
  • Took 0 min 0 sec . View more details here
Google Storage Download
  • Took 0 min 0 sec . View more details here

🤖 GitHub comments

Expand to view the GitHub comments

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

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented Jan 12, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (3/3) 💚
Files 100.0% (4/4) 💚
Classes 100.0% (4/4) 💚
Methods 60.759% (48/79) 👍
Lines 99.527% (2736/2749) 👍
Conditionals 100.0% (0/0) 💚

@andrewkroh andrewkroh changed the title Replace TSVB gauges in system metrics and host overview dashboards [System] Replace TSVB gauges in system metrics and host overview dashboards Jan 18, 2023
@drewdaemon
Copy link
Contributor Author

failure may relate to #4950

@drewdaemon drewdaemon marked this pull request as ready for review January 23, 2023 14:26
@drewdaemon drewdaemon requested review from a team as code owners January 23, 2023 14:26
@drewdaemon drewdaemon requested review from faec and leehinman January 23, 2023 14:26
Copy link
Contributor

@leehinman leehinman left a comment

Choose a reason for hiding this comment

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

Overall looks good, just some questions on the queries, especially "NOT system.network.name : l*"

@@ -8,7 +8,7 @@
"highlightAll": true,
"query": {
"language": "kuery",
"query": ""
"query": "host.name:\"docker-fleet-agent\""
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supposed to be here? I don't think this dashboard is limited to docker-fleet-agent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gvnmagni : do you have any comment on this feedback? Could this be something that snuck in while we were trying things out?

Copy link

Choose a reason for hiding this comment

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

honestly, I don't know. When we updated these visualizations we tried to replicate what was happening with the previous version of them and given my lack of competences on how data is structured behind these chart I had to intensively rely for help on Joe Reuter, who I believe originally made those. So sometimes I just followed instructions and I can't actually remember if these queries came from him or this is just an error (which would be weird, I have been very careful in not creating anything that could be unexpected).

Same happens for the next comments, the ones related to the "NOT system.network.name" query. Is it possible that is was made to avoid having dirty data of some sort? What does happen to the values if we remove these queries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, @gvnmagni ! Don't worry, I will evaluate the queries here and address this feedback. Thanks for your help. 🙏

"internalReferences": [],
"query": {
"language": "kuery",
"query": "NOT system.network.name : l*"
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this query for?

"language": "kuery",
"query": "data_stream.dataset : \"system.network\""
"query": "NOT system.network.name : l*"
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this query for?

"internalReferences": [],
"query": {
"language": "kuery",
"query": "NOT system.network.name : l*"
Copy link
Contributor

Choose a reason for hiding this comment

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

and this query?

"internalReferences": [],
"query": {
"language": "kuery",
"query": "NOT system.network.name : l*"
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

"language": "kuery",
"query": "data_stream.dataset : \"system.memory\""
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still want this query?

"language": "kuery",
"query": "data_stream.dataset : \"system.cpu\""
"query": ""
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still want this query?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is key that we prefilter on dataset. Same above.

@drewdaemon
Copy link
Contributor Author

@joshdover this PR currently changes nothing about how these metrics are computed. They will still suffer from #1437.

I would like to use this as an opportunity to choose one of the more consistent strategies I mentioned here. That will reduce user confusion in the long run.

How do I get an answer as to which strategy makes sense?

@botelastic
Copy link

botelastic bot commented Mar 11, 2023

Hi! We just realized that we haven't looked into this PR in a while. We're sorry! We're labeling this issue as Stale to make it hit our filters and make sure we get back to it as soon as possible. In the meantime, it'd be extremely helpful if you could take a look at it as well and confirm its relevance. A simple comment with a nice emoji will be enough :+1. Thank you for your contribution!

@botelastic botelastic bot added the Stalled label Mar 11, 2023
@botelastic
Copy link

botelastic bot commented Apr 10, 2023

Hi! This PR has been stale for a while and we're going to close it as part of our cleanup procedure. We appreciate your contribution and would like to apologize if we have not been able to review it, due to the current heavy load of the team. Feel free to re-open this PR if you think it should stay open and is worth rebasing. Thank you for your contribution!

@botelastic botelastic bot closed this Apr 10, 2023
@drewdaemon drewdaemon reopened this Jun 27, 2023
@drewdaemon drewdaemon requested a review from a team as a code owner June 27, 2023 17:17
@botelastic botelastic bot removed the Stalled label Jun 27, 2023
@drewdaemon drewdaemon marked this pull request as draft June 27, 2023 17:17
@botelastic
Copy link

botelastic bot commented Jul 27, 2023

Hi! We just realized that we haven't looked into this PR in a while. We're sorry! We're labeling this issue as Stale to make it hit our filters and make sure we get back to it as soon as possible. In the meantime, it'd be extremely helpful if you could take a look at it as well and confirm its relevance. A simple comment with a nice emoji will be enough :+1. Thank you for your contribution!

@botelastic botelastic bot added the Stalled label Jul 27, 2023
@botelastic
Copy link

botelastic bot commented Aug 26, 2023

Hi! This PR has been stale for a while and we're going to close it as part of our cleanup procedure. We appreciate your contribution and would like to apologize if we have not been able to review it, due to the current heavy load of the team. Feel free to re-open this PR if you think it should stay open and is worth rebasing. Thank you for your contribution!

@botelastic botelastic bot closed this Aug 26, 2023
@ruflin
Copy link
Contributor

ruflin commented Aug 28, 2023

Reopening as I'm not sure if these changes should still make it in. @joshdover

@ruflin ruflin reopened this Aug 28, 2023
@botelastic botelastic bot removed the Stalled label Aug 28, 2023
@botelastic
Copy link

botelastic bot commented Sep 27, 2023

Hi! We just realized that we haven't looked into this PR in a while. We're sorry! We're labeling this issue as Stale to make it hit our filters and make sure we get back to it as soon as possible. In the meantime, it'd be extremely helpful if you could take a look at it as well and confirm its relevance. A simple comment with a nice emoji will be enough :+1. Thank you for your contribution!

@botelastic botelastic bot added the Stalled label Sep 27, 2023
@drewdaemon
Copy link
Contributor Author

Closing in favor of #6743

@botelastic botelastic bot removed the Stalled label Sep 27, 2023
@drewdaemon drewdaemon closed this Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Integration:system System
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants