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

[Monitoring] Fixed internal monitoring check #79241

Merged
merged 16 commits into from
Oct 8, 2020

Conversation

igoristic
Copy link
Contributor

Resolves #79135

This was due to the addition of metricbeat-* indices, since their _index field does not have the -mb- marker

@igoristic igoristic added Team:Monitoring Stack Monitoring team v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Oct 2, 2020
@igoristic igoristic requested a review from a team October 2, 2020 01:11
@elasticmachine
Copy link
Contributor

Pinging @elastic/stack-monitoring (Team:Monitoring)

Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

This isn't part of this PR, but why don't we have a range query on this: https://github.com/elastic/kibana/pull/79241/files#diff-e856f71a4bdab161dc03994cf4d8317fR21?

If the user has migrated from legacy to Metricbeat collection, they will still have their old legacy indices around for at least 7 days. Maybe we should only look back one day or something? Or maybe we look at the timestamp of the last legacy collected document and determine if it's enough time back, and we see a -mb- one too?

@igoristic
Copy link
Contributor Author

@elasticmachine merge upstream

@@ -69,9 +69,9 @@ export function monitoringClustersProvider($injector) {
if (Legacy.shims.isCloud) {
return Promise.resolve();
}

const css = window.location.hash.split('?')[0] === '#/home' ? 'css' : 'single';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a big fan of hard-coding a URL when I think we can manage without it.

monitoringClusters is only called from a few places, and we can just provide an additional parameter to the function call for ccs and we can specify which one from the invokers.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chrisronline Actually this won't work. I tried passing it here:

return monitoringClusters(undefined, undefined, CODE_PATHS);
but looks like monitoringClusters executes somewhere else before hand (probably in a routeInit), which sets the once variable before we can check the param. This means I would have to do some extra logic to determine the "real" origin (not as clean as the one liner above)

Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

LGTM!

@igoristic
Copy link
Contributor Author

@elasticmachine merge upstream

@chrisronline
Copy link
Contributor

Hold up on merging this. I'm testing this again and seeing some unexpected behavior. I'll update when I know more

@@ -69,9 +69,9 @@ export function monitoringClustersProvider($injector) {
if (Legacy.shims.isCloud) {
return Promise.resolve();
}

const ccs = window.location.hash.split('?')[0] === '#/home' ? 'ccs' : 'single';
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this, why don't we just get the globalState and add globalState.ccs directly? The prefixIndexPattern code is designed to accept a ccs string like this

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
monitoring 1.2MB 1.2MB +68.0B

History

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

Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

LGTM!

@igoristic igoristic merged commit cd4df4b into elastic:master Oct 8, 2020
@igoristic igoristic deleted the internal_check_fix branch October 8, 2020 19:54
@igoristic igoristic added v7.11.0 and removed v7.10.0 labels Oct 8, 2020
igoristic added a commit that referenced this pull request Oct 8, 2020
* fixed internal monitoring check

* Added range filter

* Added single vs ccs condtion

* Fixed spelling

* Passing global state ccs

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@igoristic
Copy link
Contributor Author

igoristic commented Oct 8, 2020

Backport:
7.x: f61fcb8
7.10: 0379b9a

gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 12, 2020
* master: (23 commits)
  Table visualization renderer (elastic#79455)
  Migrate Jest JUnit reporter to TS (elastic#79919)
  store sorted bundleRefExportIds (elastic#80011)
  update chromedriver dependency to 86.0.0 (elastic#79972)
  [Security Solution][Case] Fix bug when changing connectors (elastic#80002)
  [kbn/std] add observable helpers to aid with rxjs 7 upgrade (elastic#79752)
  [Security Solution][Resolver] Pill numbers in compact notation (elastic#80038)
  [Logs UI] Sync logs timerange with wider Kibana (elastic#79444)
  [DOCS] Adds quick start (elastic#78822)
  [Ingest Manager]Fix ingest manager UI renaming (elastic#80036)
  [Monitoring] Fixed internal monitoring check (elastic#79241)
  [Security Solution][Exception Modal] Removes list operators in exception modal for EQL rules (elastic#79871)
  Update development documentation about REST API best practices (elastic#80009)
  [Monitoring] Improve indices loading against larger metricbeat-* indices (elastic#79190)
  [CI] Move kibana build dir outside of repo for functional tests (elastic#80018)
  [kbn/optimizer] bump low or add missing limits when updating automatically (elastic#80013)
  [Enterprise Search] Added a Credentials page to App Search (elastic#79749)
  [DOCS] Canvas refresh for 7.10 (elastic#80017)
  [TSVB] Add ignore global filters to series options (elastic#79337)
  Remove this check (elastic#79202)
  ...
igoristic added a commit that referenced this pull request Oct 28, 2020
* fixed internal monitoring check

* Added range filter

* Added single vs ccs condtion

* Fixed spelling

* Passing global state ccs

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	x-pack/plugins/monitoring/public/services/clusters.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported release_note:skip Skip the PR/issue when compiling release notes Team:Monitoring Stack Monitoring team v7.10.0 v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Partial legacy monitoring messages shows up despite no legacy present
4 participants