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

Disable the experimental metrics_entities plugin by default. #115460

Merged

Conversation

oatkiller
Copy link
Contributor

@oatkiller oatkiller commented Oct 18, 2021

Summary

Disable the experimental metrics_entities plugin by default. This is required due to the following change: #89584

TODO

  • The items on the checklist and risk matrix need to be addressed.
  • Validate that the plugin is correctly disabled by default
  • Validate that the plugin now works when enabled via configuration key.

Checklist

  • Documentation was added for features that require explanation or tutorials
    • We'll defer this decision until after FF.
  • Unit or functional tests were updated or added to match the most common scenarios
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list

Risk Matrix

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
This plugin was enabled by default in past releases (I assume?) Is this a breaking change? High The plugin adds HTTP routes that do CRUD on transforms. These routes would be unavailable until the plugin was re-enabled This may be irrelevant as the plugin is experimental, but we don't have telemetry to back that up. Do we have any known users of this plugin that we could communicate with?

For maintainers

Copy link
Contributor

@yctercero yctercero left a comment

Choose a reason for hiding this comment

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

RE: the matrix question. This plugin was disabled by default previously, though with the changes that had gone in around changing configs in platform, it had since become enabled by default in master. So it's my understanding that this change would just maintain the status quo for this plugin.

I noticed that rule-registry had been on the list as not needing to be disable-able. Are we a go with it for 7.16/8.0?

Wondering if there's documentation updates needed? (That can obv be post FF).

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

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

@FrankHassanabad FrankHassanabad added the release_note:skip Skip the PR/issue when compiling release notes label Oct 19, 2021
@oatkiller
Copy link
Contributor Author

@yctercero

This plugin was disabled by default previously...

From reading this comment on the original issue I understand that this code is responsible for determining if a plugin is enabled.

From my understanding of that code, I assume that metrics_entities was enabled in 7.15. Does that add up to you?

I noticed that rule-registry had been on the list as not needing to be disable-able. Are we a go with it for 7.16/8.0?
Thank you so much for looking into that. I don't know but I'll definitely check right away. Thanks!

@oatkiller
Copy link
Contributor Author

@yctercero Rule registry seems to be disabled by default.

@FrankHassanabad
Copy link
Contributor

This plugin was enabled by default in past releases (I assume?) Is this a breaking change?

No, you can see here in 7.15:
https://github.com/elastic/kibana/blob/7.15/x-pack/plugins/metrics_entities/server/config.ts#L11

Do we have any known users of this plugin that we could communicate with?
No we do not, since this is was not enabled in the past. Putting this back into disabled by default puts us to where we were in 7.15 so we should be 👍

@oatkiller
Copy link
Contributor Author

@FrankHassanabad confirmed that it was disabled by default in 7.15. Thanks!
I just did a quick code review of the plugin. It seems like enabling it doesn't do anything eagerly except register HTTP routes. When those routes are called they run ES queries using the 'asCurrentUser' authentication context. This all seems safe to me. If a user was somehow relying on this plugin, they'd get HTTP errors and then after enabling the plugin, they'd be good to go.

I think we can defer the decision to add user docs and I'll follow up with the cloud/dockerlist question after merge.

@oatkiller oatkiller merged commit b402bea into elastic:master Oct 19, 2021
@oatkiller oatkiller deleted the disable-metrics-entities-plugin-by-default branch October 19, 2021 13:33
@yctercero yctercero added Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Security Solution Platform Security Solution Platform Team labels Oct 19, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@FrankHassanabad FrankHassanabad added the auto-backport Deprecated - use backport:version if exact versions are needed label Oct 19, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
7.x Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 115460

yctercero added a commit that referenced this pull request Oct 20, 2021
…115460) (#115694)

* Disable the experimental `metrics_entities` plugin by default. (#115460)

This was default disabled in 7.15, but we needed a code change to maintain that (consistent) behavior.
# Conflicts:
#	x-pack/plugins/metrics_entities/server/index.ts

* Remove old config for metrics

* Remove unused imports

Co-authored-by: Robert Austin <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
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 release_note:skip Skip the PR/issue when compiling release notes Team:Security Solution Platform Security Solution Platform Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants