Exposing the ability to log deprecated settings at non-critical level #79107

masseyke · 2021-10-13T22:58:43Z

A recent change for the deprecation logs provided the capability to emit deprecation's at critical vs. warning levels, #77482.
However deprecated settings always log at critical level without the ability to express that the setting deprecation is only a
warning.

This commit exposes the ability to set the deprecation level when deprecating a setting.
Relates #78781

elasticmachine · 2021-10-13T22:58:46Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

masseyke · 2021-10-14T16:15:35Z

@elasticmachine update branch

…el-master

rjernst

Looks good in general, I just have a few comments about not adding more variants of methods in HeaderWarning and ESTestCase.

rjernst · 2021-10-14T20:13:16Z

server/src/main/java/org/elasticsearch/common/logging/HeaderWarning.java

+        addWarning(THREAD_CONTEXT, DeprecationLogger.CRITICAL, message, params);
+    }
+
+    public static void addWarning(Level level, String message, Object... params) {


Rather than adding another public method, can you please update the existing uses? It looks like there are ~12 calls to the existing method, and most are in tests, it should be easy to update with eg IntelliJ refactoring tools to add a parameter.

Yeah I went back and forth on this. I actually did this and backed it out in an attempt to make this PR smaller. I'll consolidate it into one method.

rjernst · 2021-10-14T20:15:59Z

server/src/main/java/org/elasticsearch/common/logging/HeaderWarning.java

@@ -193,10 +196,14 @@ private static boolean assertWarningValue(final String s, final String warningVa
     * @return a warning value formatted according to RFC 7234
     */
    public static String formatWarning(final String s) {
+        return formatWarning(DeprecationLogger.CRITICAL, s);


Another method seems unnecessary, the only non-test use is in this class, let's just update the few tests using this to pass the level?

rjernst · 2021-10-14T20:16:44Z

server/src/main/java/org/elasticsearch/common/settings/Setting.java

@@ -99,10 +99,15 @@
        Final,

        /**
-         * mark this setting as deprecated
+         * mark this setting as deprecated (critical level)
         */
        Deprecated,


It would be nice, as a followup (since it will touch so many files), to rename this.

rjernst · 2021-10-14T20:18:27Z

server/src/main/java/org/elasticsearch/common/settings/Setting.java

+    }
+
+    private boolean isDeprecatedWarningOnly() {
+        return properties.contains(Property.DeprecatedWarning) && properties.contains(Property.Deprecated) == false;


We should assert somewhere that both properties do not exist, they should be mutually exclusive. Then this method isn't needed, just check for contains the warning version?

I added this to the Setting constructor, and updated this method..

rjernst · 2021-10-14T20:19:56Z

test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java

+                Arrays.stream(warnings))
+                .toArray(String[]::new));
+    }
+
    protected final void assertWarnings(String... expectedWarnings) {
        assertWarnings(true, expectedWarnings);
    }

    protected final void assertWarnings(boolean stripXContentPosition, String... expectedWarnings) {


There are 4 uses of this method. Let's update those to call the updated signature rather than adding yet another variant.

rjernst · 2021-10-14T20:24:49Z

test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java

@@ -458,11 +460,35 @@ protected final void assertSettingDeprecationsAndWarnings(final String[] setting
                        .toArray(String[]::new));
    }

+    protected final void assertSettingDeprecationsAndWarningsIncludingLevel(final Setting<?>[] settings, final String... warnings) {


There are only 2 uses of the method above which takes string setting names. Let's collapse all 3 of these methods into one, this is unwieldy having 3 methods of the same name, with no clear distinction of when to call each.

This is another one I'd gone back and forth on. I've merged them all into one method now.

… github.com:masseyke/elasticsearch into feature/deprecated-settings-at-warning-level-master

masseyke · 2021-10-15T14:41:35Z

@elasticmachine update branch

elasticmachine · 2021-10-15T14:41:37Z

merge conflict between base and head

…el-master

rjernst

Looks good, but one last question

rjernst · 2021-10-15T18:17:36Z

test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java

-     *
-     * @param settings the settings that are expected to be deprecated
+     *  @param settings the settings that are expected to be deprecated
+     * @param shouldAssertExpectedLogLevel


I removed this param (since it's no longer optional).

rjernst · 2021-10-15T18:18:12Z

test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java

-    }
-
-    protected final void assertSettingDeprecationsAndWarnings(final String[] settings, final String... warnings) {
+    protected final void assertSettingDeprecationsAndWarnings(final Setting<?>[] settings, boolean shouldAssertExpectedLogLevel,


Sorry I missed this before, I thought it was just settings and warnings, why wouldn't we always assert the expected log level?

Yeah no real harm in checking it for everything, and it might be useful in the future. I've added that.

rjernst

LGTM

* upstream/master: Validate tsdb's routing_path (elastic#79384) Adjust the BWC version for the return200ForClusterHealthTimeout field (elastic#79436) API for adding and removing indices from a data stream (elastic#79279) Exposing the ability to log deprecated settings at non-critical level (elastic#79107) Convert operator privilege license object to LicensedFeature (elastic#79407) Mute SnapshotBasedIndexRecoveryIT testSeqNoBasedRecoveryIsUsedAfterPrimaryFailOver (elastic#79456) Create cache files with CREATE_NEW & SPARSE options (elastic#79371) Revert "[ML] Use a new annotations index for future annotations (elastic#79151)" [ML] Use a new annotations index for future annotations (elastic#79151) [ML] Removing legacy code from ML/transform auditor (elastic#79434) Fix rate agg with custom `_doc_count` (elastic#79346) Optimize SLM Policy Queries (elastic#79341) Fix execution of exists query within nested queries on field with doc_values disabled (elastic#78841) Stricter UpdateSettingsRequest parsing on the REST layer (elastic#79227) Do not release snapshot file download permit during recovery retries (elastic#79409) Preserve request headers in a mixed version cluster (elastic#79412) Adjust versions after elastic#79044 backport to 7.x (elastic#79424) Mute BulkByScrollUsesAllScrollDocumentsAfterConflictsIntegTests (elastic#79429) Fail on SSPL licensed x-pack sources (elastic#79348) # Conflicts: # server/src/test/java/org/elasticsearch/index/TimeSeriesModeTests.java

As #79107 of the ESTestCase assertWarnings methods now check that the log level is of the expected type. This commit puts in the correct expected log level. Relates #79107 #78921

…elastic#79107) A recent change for the deprecation logs provided the capability to emit deprecation's at critical vs. warning levels, elastic#77482. However deprecated settings always log at critical level without the ability to express that the setting deprecation is only a warning. This commit exposes the ability to set the deprecation level when deprecating a setting. Closes elastic#78781

…79107) (#79492) A recent change for the deprecation logs provided the capability to emit deprecation's at critical vs. warning levels, #77482. However deprecated settings always log at critical level without the ability to express that the setting deprecation is only a warning. This commit exposes the ability to set the deprecation level when deprecating a setting. Relates #78781

… in 8.0 (#79665) This commit lowers the severity for all settings from critical to warning for all settings that are still present in 8.0.0. releated #79107

This commit changes the deprecation logger so that all messages (critical or warning) are written out with "299" as the level at the beginning of the header in order to be compliant with https://www.rfc-editor.org/rfc/rfc7234.html#section-5.5.7. In #79107 we mistakenly began logging warning-level messages with the 300 code, which is not valid in the RFC.

…#80304) This commit changes the deprecation logger so that all messages (critical or warning) are written out with "299" as the level at the beginning of the header in order to be compliant with https://www.rfc-editor.org/rfc/rfc7234.html#section-5.5.7. In elastic#79107 we mistakenly began logging warning-level messages with the 300 code, which is not valid in the RFC.

…#80501) This commit changes the deprecation logger so that all messages (critical or warning) are written out with "299" as the level at the beginning of the header in order to be compliant with https://www.rfc-editor.org/rfc/rfc7234.html#section-5.5.7. In #79107 we mistakenly began logging warning-level messages with the 300 code, which is not valid in the RFC.

…#80503) * Changing HeaderWarning to always use 299 as the warning code (#80304) This commit changes the deprecation logger so that all messages (critical or warning) are written out with "299" as the level at the beginning of the header in order to be compliant with https://www.rfc-editor.org/rfc/rfc7234.html#section-5.5.7. In #79107 we mistakenly began logging warning-level messages with the 300 code, which is not valid in the RFC. * fixing compilation errors from merge

Exposing the ability to log deprecated settings at non-critical level

b1dfda3

masseyke added the :Core/Infra/Logging Log management and logging utilities label Oct 13, 2021

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Oct 13, 2021

masseyke added v7.16.0 v8.0.0 labels Oct 13, 2021

masseyke added 2 commits October 13, 2021 18:33

replacing a previously existing method

19788f4

fixing tests

d5884c6

Merge branch 'master' into feature/deprecated-settings-at-warning-lev…

b7b5f75

…el-master

rjernst reviewed Oct 14, 2021

View reviewed changes

masseyke added 4 commits October 14, 2021 18:08

code review feedback incorporated

e25078d

Merge branch 'feature/deprecated-settings-at-warning-level-master' of…

0dd2666

… github.com:masseyke/elasticsearch into feature/deprecated-settings-at-warning-level-master

code review feedback incorporated

4ac690d

incorporating code review feedback

d8c8dde

masseyke added 4 commits October 15, 2021 09:54

Merge branch 'master' into feature/deprecated-settings-at-warning-lev…

c90725b

…el-master

getting rid of compiler warning

bebb895

fixing a checkstyle problem

841fbb2

fixing a checkstyle problem

d2a82a7

masseyke requested a review from rjernst October 15, 2021 17:05

rjernst reviewed Oct 15, 2021

View reviewed changes

masseyke added 3 commits October 18, 2021 16:12

Checking log level for all deprecation warnings

0c18846

fixing compilation errors / cleaning up

9fa7f60

fixing a unit test

0bb8894

masseyke requested a review from rjernst October 18, 2021 23:13

rjernst approved these changes Oct 18, 2021

View reviewed changes

masseyke merged commit 88250c2 into elastic:master Oct 19, 2021

masseyke added the backport pending label Oct 19, 2021

masseyke mentioned this pull request Oct 19, 2021

Fixing a unit test #79470

Merged

masseyke added a commit that referenced this pull request Oct 19, 2021

Fixing a unit test (#79470)

9bec026

As #79107 of the ESTestCase assertWarnings methods now check that the log level is of the expected type. This commit puts in the correct expected log level. Relates #79107 #78921

jakelandis mentioned this pull request Oct 21, 2021

Reduce deprecation logging severity for settings that are not removed in 8.0 #79665

Merged

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

masseyke removed the backport pending label Oct 27, 2021

masseyke deleted the feature/deprecated-settings-at-warning-level-master branch November 2, 2021 17:48

masseyke mentioned this pull request Nov 4, 2021

Changing HeaderWarning to always use 299 as the warning code #80304

Merged

masseyke mentioned this pull request Nov 8, 2021

Changing HeaderWarning to always use 299 as the warning code (#80304) #80503

Merged

danhermann added the >deprecation label Dec 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exposing the ability to log deprecated settings at non-critical level #79107

Exposing the ability to log deprecated settings at non-critical level #79107

masseyke commented Oct 13, 2021 •

edited

Loading

elasticmachine commented Oct 13, 2021

masseyke commented Oct 14, 2021

rjernst left a comment

rjernst Oct 14, 2021

masseyke Oct 14, 2021

rjernst Oct 14, 2021

rjernst Oct 14, 2021

rjernst Oct 14, 2021

masseyke Oct 15, 2021

rjernst Oct 14, 2021

masseyke Oct 15, 2021

rjernst Oct 14, 2021

masseyke Oct 15, 2021

masseyke commented Oct 15, 2021

elasticmachine commented Oct 15, 2021

rjernst left a comment

rjernst Oct 15, 2021

masseyke Oct 18, 2021

rjernst Oct 15, 2021

masseyke Oct 18, 2021

rjernst left a comment

Exposing the ability to log deprecated settings at non-critical level #79107

Exposing the ability to log deprecated settings at non-critical level #79107

Conversation

masseyke commented Oct 13, 2021 • edited Loading

elasticmachine commented Oct 13, 2021

masseyke commented Oct 14, 2021

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masseyke commented Oct 15, 2021

elasticmachine commented Oct 15, 2021

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst left a comment

Choose a reason for hiding this comment

masseyke commented Oct 13, 2021 •

edited

Loading