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

[Stack Monitoring] Add stack_monitoring suffix to metrics-* index pattern #137904

Merged

Conversation

crespocarlos
Copy link
Contributor

@crespocarlos crespocarlos commented Aug 2, 2022

Summary

This PR closes #135382 by updating metrics-*index pattern - which are generated by the integration packages -, adding stack_monitoring to it, so that the UI can reflect this update on the packages

It prepares the SM server code for the index pattern update that will happen on the SM integration packages

For maintainers

There is actually a limit of 255 characters for index names for index name, but I don't expect that adding this suffix will cause problems in the future. Considering the following example:

.ds-metrics-elasticsearch.index_recovery-default-2022.08.02-000001 = 66 characters


This PR can only be merged once the integration packages indices have been updated

@crespocarlos crespocarlos changed the title Add stack_monitoring to metrics- index pattern [Stack Monitoring] Add stack_monitoring to metrics-* index pattern Aug 2, 2022
@crespocarlos crespocarlos changed the title [Stack Monitoring] Add stack_monitoring to metrics-* index pattern [Stack Monitoring] Add stack_monitoring suffix to metrics-* index pattern Aug 2, 2022
@crespocarlos crespocarlos added Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes Feature:Stack Monitoring v8.5.0 backport:skip This commit does not require backporting labels Aug 2, 2022
@crespocarlos crespocarlos marked this pull request as ready for review August 3, 2022 09:46
@crespocarlos crespocarlos requested a review from a team as a code owner August 3, 2022 09:46
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

@neptunian
Copy link
Contributor

Looks good. Just a note that until #123508 is complete, there are still a few places that won't be updated yet.

@matschaffer
Copy link
Contributor

@elasticmachine merge upstream

@crespocarlos
Copy link
Contributor Author

buildkite test this

@matschaffer
Copy link
Contributor

@elasticmachine merge upstream

@matschaffer
Copy link
Contributor

Looks like just a docker blip on the last run so re-updating

@klacabane
Copy link
Contributor

Should we block this PR until mappings alignment is complete (only logstash left elastic/integrations#3993) ? We can then update the package's data streams to write to the new .stack_monitoring. dataset in elastic/integrations#3929 and verify the changes against this PR ?

@klacabane
Copy link
Contributor

klacabane commented Aug 17, 2022

When updating a data stream's dataset, not only the index name changes but also the data_stream.dataset property that we use to target the correct documents. I caught that while testing elastic/integrations#3929
I added the change in ac818d4

@klacabane
Copy link
Contributor

@elasticmachine merge upstream

@@ -123,7 +123,9 @@ export function getDsIndexPattern({
config,
ccs,
}: CommonIndexPatternArgs & { type?: string }): string {
const datasetsPattern = `${moduleType ?? '*'}.${dataset ?? '*'}`;
const datasetsPattern = `${moduleType ?? '*'}.${
type === DS_INDEX_PATTERN_METRICS ? 'stack_monitoring.' : ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Added this condition when resolving conflicts and looks like this function is doing too much. We could split it in getMetricsIndexPatterns and getLogsIndexPatterns. the split itself is reasonable and makes more sense with the dataset branching now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @klacabane. I'll try to improve this.

@crespocarlos
Copy link
Contributor Author

@elasticmachine merge upstream

@matschaffer
Copy link
Contributor

Pretty sure we're still holding this PR, so I added the blocked label to make sure I'm reviewing things in a good order :)

@matschaffer
Copy link
Contributor

matschaffer commented Sep 6, 2022

When testing elastic/integrations#4018 I found that this branch produces a "ghost" standalone cluster when logstash connection errors are present.

Screen Shot 2022-09-06 at 15 23 46

Screen Shot 2022-09-06 at 15 25 55

Could be that's out of scope for this PR, but it'd be a little weird to ship kibana with this behavior (assuming we haven't already).

Update: elastic/integrations#4011 is open to fix that so we probably can skip it for this PR

@klacabane
Copy link
Contributor

The ghost standalone will be fixed with #140102 and elastic/integrations#4147

@crespocarlos
Copy link
Contributor Author

Should we wait those fixes before merging this one?

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@klacabane
Copy link
Contributor

klacabane commented Sep 7, 2022

I'm inclined to merge this now since the corresponding integration PR was merged, and the standalone issue does not impact metricbeat/legacy collection mode.

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.

Agreed. I've been using this to test the standalone changes and it seems great.

@klacabane klacabane merged commit 051e9b7 into elastic:main Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting blocked Feature:Stack Monitoring release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Stack Monitoring] Update metrics-* index pattern
7 participants