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

[Metrics UI] Filter out APM nodes from the inventory view #110300

Merged
merged 4 commits into from
Sep 3, 2021

Conversation

Zacqary
Copy link
Contributor

@Zacqary Zacqary commented Aug 26, 2021

Summary

Closes #107565

Filters out APM nodes from the Inventory view, as they don't report the correct set of metrics to be useful.

The Snapshot API will filter out any nodes that:

  • Have the current value as null
  • Have the metricset name app, which indicates that this is data from APM and not Metricbeat

Note that this PR will NOT filter out any APM nodes that do successfully report data. When APM is updated to report data compatible with the Inventory view, we won't need to make any changes to the Metrics app for it to work.

Checklist

@Zacqary Zacqary added release_note:fix Feature:Metrics UI Metrics UI feature v8.0.0 Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.16.0 labels Aug 26, 2021
@Zacqary Zacqary requested review from sorenlouv and a team August 26, 2021 17:28
return { ...node, path, name };
});
const isNoData = node.metrics.every((m) => m.value === null);
const isAPMNode = series.metricsets?.includes('app');
Copy link
Member

Choose a reason for hiding this comment

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

APM produces other metric sets than app so I'm not sure this check will filter all APM nodes out.
Perhaps it would be better to check for the existence of one or more required fields, eg the cpu value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there any APM nodes that don't produce app? series.metricsets includes every metric set attached to the node in question, so as long as app is present then the existence of other sets don't matter.

I'm not sure how else to differentiate between a missing CPU value because of a No Data situation, and a missing CPU value because the node is from APM.

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

lgtm

@Zacqary Zacqary marked this pull request as ready for review September 2, 2021 15:51
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@Zacqary Zacqary added the auto-backport Deprecated - use backport:version if exact versions are needed label Sep 2, 2021
@Zacqary Zacqary enabled auto-merge (squash) September 2, 2021 15:51
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
infra 1.7MB 1.7MB +384.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@sorenlouv
Copy link
Member

@Zacqary Any chance this can make it to 7.15?

Copy link
Contributor

@matschaffer matschaffer 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 wonder if we might have some more work to do to avoid displaying duplicates if the data gets collected by both apm & metricbeat, as @alex-fedotyev mentioned here.

Not sure if the code accounts for it, but I don't see a test for it so I'm guessing not.

Either way I think we can worry about that once APM is able to send the expected data.

@Zacqary Zacqary merged commit a99360f into elastic:master Sep 3, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 3, 2021
…0300)

* [Metrics UI] Filter out APM nodes from the inventory view

* Update jest snapshots

* Add tests for fs for filtering out APM nodes
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Sep 3, 2021
…111074)

* [Metrics UI] Filter out APM nodes from the inventory view

* Update jest snapshots

* Add tests for fs for filtering out APM nodes

Co-authored-by: Zacqary Adam Xeper <[email protected]>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 3, 2021
…eporting-to-v2

* 'master' of github.com:elastic/kibana: (65 commits)
  Move to vis_types folder part 2 (elastic#110574)
  [SOR] use initialNamespaces when checking for conflict for `create` and `bulkCreate` (elastic#111023)
  [Discover] Remove export* syntax (elastic#110934)
  [Event log][7.x] Updated event log client to search across legacy IDs (elastic#109365)
  [Security Solution][Detection Rules] Changes 'activated' text on rule details page  (elastic#111044)
  [Metrics UI] Filter out APM nodes from the inventory view (elastic#110300)
  [package testing] Update logging and pid configuration (elastic#111059)
  [Dashboard] Read App State from URL on Soft Refresh (elastic#109354)
  Add correct roles to test user for functional tests in dashboard (elastic#110880)
  [DOCS] Adds Lens Inspector and minor edits (elastic#109736)
  [DOCS] Updates Spaces page (elastic#111005)
  normalize initialNamespaces (elastic#110936)
  [Reporting] Clean up `any` usage, reorganize server route files (elastic#110740)
  [Security Solution] [CTI] Fixes bug that caused Threshold and Indicator Match rules to ignore custom rule filters if a saved query was used in the rule definition. (elastic#109253)
  skip flaky suites: elastic#111001, elastic#111022
  [Security Solution][RAC] - Update reason field text (elastic#110308)
  [RAC][Security Solution] Make analyzer work with EuiDataGrid full screen (elastic#110913)
  [Metrics UI] Add integration tests for Metric Threshold Rule and refactor to fire correctly (elastic#109971)
  [DOCS] Updates Discover docs (elastic#110346)
  [RAC] Persistent timeline fields fix (elastic#110685)
  ...
@Zacqary Zacqary added the v7.15.1 label Sep 3, 2021
Zacqary added a commit to Zacqary/kibana that referenced this pull request Sep 3, 2021
…0300)

* [Metrics UI] Filter out APM nodes from the inventory view

* Update jest snapshots

* Add tests for fs for filtering out APM nodes
Zacqary added a commit that referenced this pull request Sep 3, 2021
…111142)

* [Metrics UI] Filter out APM nodes from the inventory view

* Update jest snapshots

* Add tests for fs for filtering out APM nodes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Metrics UI Metrics UI feature release_note:fix Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.15.1 v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reflect APM collected metrics in Observability Overview
5 participants