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

Add deprecation info API entries for deprecated monitoring settings #78799

Merged
merged 9 commits into from
Oct 19, 2021

Conversation

jbaiera
Copy link
Member

@jbaiera jbaiera commented Oct 6, 2021

Recently we have deprecated a number of settings in monitoring. These settings should be represented in the deprecation info API. This PR will be backported with some minor changes to the 7.x branch so that we can start the deprecation process in that release cycle.

Related issues:
#77659
#78564

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@jbaiera jbaiera requested a review from jrodewig October 6, 2021 22:22
@jrodewig
Copy link
Contributor

jrodewig commented Oct 7, 2021

Tagging @debadair as she's working on best practices for the deprecation API messages.

Thanks for ensuring these deprecations get added, @jbaiera. However, I'm confused about why this PR targets master. Shouldn't the deprecations only apply to 7.x branches?

@jrodewig jrodewig requested a review from debadair October 7, 2021 17:33
@danhermann
Copy link
Contributor

Thanks for ensuring these deprecations get added, @jbaiera. However, I'm confused about why this PR targets master. Shouldn't the deprecations only apply to 7.x branches?

We typically target the master branch for deprecations and then backport to the branch where the deprecation will first take effect. In some cases, the deprecated feature will then be removed from the master branch but we're increasingly using longer deprecation periods in which case the deprecated feature will remain deprecated even in the master branch.

Copy link
Contributor

@danhermann danhermann left a comment

Choose a reason for hiding this comment

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

Code changes LGTM

@jrodewig
Copy link
Contributor

jrodewig commented Oct 7, 2021

Thanks for the explanation @danhermann.

Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

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

Thanks again @jbaiera. I'll leave final docs approval to @debadair. I think there's an opportunity to improve the boilerplate text here, but she's in a better position to judge that.

Otherwise, I think we need to remove the new sentence from the breaking changes docs.

Copy link
Contributor

@debadair debadair left a comment

Choose a reason for hiding this comment

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

I agree with the patterns James suggested for the case where there are multiple settings. Had one question about the breaking changes entry--it contradicts itself. :-)

The `xpack.monitoring.exporters.*.use_ingest` property was deprecated in 7.16.0 and
has been removed. This parameter controlled the creation of pipelines for monitoring
indices that previously had no function.
The `xpack.monitoring.exporters.*.use_ingest` property was deprecated in 7.16.0. This
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 setting going to be removed in 8.0, or after 8.0? The entry says it's removed, the Details say it will be removed. If it's only deprecated, I don't think we should include it in the breaking changes.

Copy link
Member

Choose a reason for hiding this comment

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

We have similar ambiguous documentation of "index.template.create_legacy_templates" (a deprecation info message for it was added in this PR, but the documentation has been ambiguous for a while).

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the header text. A lot of this is all messed up because we made a recent course correction to not remove these settings in 8.0. So much copy pasta to clean up.

Copy link
Contributor

Choose a reason for hiding this comment

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

If they are going to remain deprecated, I think we can just doc them as deprecated in 7.16 and omit them from the 8.0 breaking changes. The typical pattern is to only include newly-deprecated things, not repeat everything that's been previously deprecated.

Copy link
Contributor

@jrodewig jrodewig Oct 19, 2021

Choose a reason for hiding this comment

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

If they are going to remain deprecated, I think we can just doc them as deprecated in 7.16 and omit them from the 8.0 breaking changes.

+1. Let's remove these from 8.0. However, we'll still need to update the 7.16 deprecation docs to remove references to an 8.0 removal.

Copy link
Member

@masseyke masseyke left a comment

Choose a reason for hiding this comment

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

Looks good to me, although I share @debadair's slight confusion about the documentation.

The `xpack.monitoring.exporters.*.use_ingest` property was deprecated in 7.16.0 and
has been removed. This parameter controlled the creation of pipelines for monitoring
indices that previously had no function.
The `xpack.monitoring.exporters.*.use_ingest` property was deprecated in 7.16.0. This
Copy link
Member

Choose a reason for hiding this comment

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

We have similar ambiguous documentation of "index.template.create_legacy_templates" (a deprecation info message for it was added in this PR, but the documentation has been ambiguous for a while).

Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

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

The deprecation info API changes LGTM. However, as @debadair pointed out, we should remove the 8.0 breaking changes docs. Instead, we should update the 7.16 deprecations to update any 8.0 removal references, probably as a separate PR.

Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for removing the docs.

@jbaiera
Copy link
Member Author

jbaiera commented Oct 19, 2021

@elasticmachine update branch

@jbaiera jbaiera merged commit ceaf53c into elastic:master Oct 19, 2021
@jbaiera jbaiera deleted the monitoring-deprecation-info-additions branch October 19, 2021 16:50
jbaiera added a commit to jbaiera/elasticsearch that referenced this pull request Oct 20, 2021
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Oct 20, 2021
* upstream/master: (24 commits)
  Implement framework for migrating system indices (elastic#78951)
  Improve transient settings deprecation message (elastic#79504)
  Remove getValue and getValues from Field (elastic#79516)
  Store Template's mappings as bytes for disk serialization (elastic#78746)
  [ML] Add queue_capacity setting to start deployment API (elastic#79433)
  [ML] muting rest compat test issue elastic#79518 (elastic#79519)
  Avoid redundant available indices check (elastic#76540)
  Re-enable BWC tests
  TEST Ensure password 14 chars length on Kerberos FIPS tests (elastic#79496)
  [DOCS] Temporarily remove APM links (elastic#79411)
  Fix CCSDuelIT for skipped shards (elastic#79490)
  Add other time accounting in HotThreads (elastic#79392)
  Add deprecation info API entries for deprecated monitoring settings (elastic#78799)
  Add note in breaking changes for nameid_format (elastic#77785)
  Use 'migration' instead of 'upgrade' in GET system feature migration status responses (elastic#79302)
  Upgrade lucene version 8b68bf60c98 (elastic#79461)
  Use Strings#EMPTY_ARRAY (elastic#79452)
  Quicker shared cache file preallocation (elastic#79447)
  [ML] Removing some code that's obsolete for 8.0 (elastic#79444)
  Ensure indexing_data CCR requests are compressed (elastic#79413)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants