Change default level for deprecation logging by jakelandis · Pull Request #80167 · elastic/elasticsearch

jakelandis · 2021-11-01T22:35:28Z

This commit updates all deprecation message (except for REST
compatible API messages) in 8.0+ to be emit at warning level.
Currently none of these have been removed in future versions (yet) so they
should be logged at warning, not critical.

This commit also changes the default assertWarning to assert at warning level
and introduces a new assertCriticalWarning to assert critical warnings.

pgomulka

LGTM

jakelandis · 2021-11-02T14:28:03Z

@pgomulka - can you take another look ? I also drop the non-settings down to warn level, such that ~~nothing~~ only REST compatible warning will currently log to critical. It looks like like of changes, but the code change was just some creative use of Intellij's refactoring.

pgomulka

LGTM, I scrolled through the changes and all looks great

…critical

jakelandis · 2021-11-02T15:02:00Z

libs/x-content/src/test/java/org/elasticsearch/xcontent/ConstructingObjectParserTests.java

            StructRemovalField parse = StructRemovalField.PARSER.parse(parser, null);

-            assertWarnings("The field old_name has been removed and is being ignored");
+            assertCriticalWarnings("The field old_name has been removed and is being ignored");


REST API compatibility still warns at critical level.

I guess it is ok? compatibility warns about a feature that was already removed

jakelandis · 2021-11-02T15:06:11Z

server/src/main/java/org/elasticsearch/rest/DeprecationRestHandler.java

    public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
        if (compatibleVersionWarning == false) {
-            deprecationLogger.critical(DeprecationCategory.API, deprecationKey, deprecationMessage);
+            deprecationLogger.warn(DeprecationCategory.API, deprecationKey, deprecationMessage);


We will still need a way to emit critical level deprecations when decalaring a REST handler. I will log a follow up issue.

jakelandis · 2021-11-03T17:39:36Z

...ion/qa/rest/src/javaRestTest/java/org/elasticsearch/xpack/deprecation/DeprecationHttpIT.java

+    /**
+     * Check that a deprecation message with CRITICAL level can be recorded to an index
+     */
+    public void testDeprecationCriticalWarnMessagesCanBeIndexed() throws Exception {


Now that the default is WARN, we needed to cover the case of a non-compat critical warning.

jakelandis · 2021-11-03T17:59:57Z

@elasticmachine update branch

masseyke · 2021-11-03T19:08:01Z

I believe all of your test failures are fixed by #80304 if we go that direction.

jakelandis · 2021-11-04T13:46:28Z

@elasticmachine update branch

jakelandis · 2021-11-04T14:44:34Z

@dakrone - can you take a look, this PR should be stable now. The initial (and primary change) was done with Intellij refactoring, however some the tests needed to updated to account for the new default warning level.

dakrone

LGTM

This commit updates all deprecation message (except for REST compatible API messages) in 8.0+ to be emit at warning level. Currently none of these have been removed in future versions (yet) so they should be logged at warning, not critical. This commit also changes the default assertWarning to assert at warning level and introduces a new assertCriticalWarning to assert critical warnings.

This allows specifying an optional level to be used for the deprecation handler registering a route. Relates to elastic#80167

This allows specifying an optional level to be used for the deprecation handler registering a route. The default remains if the level is unspecified. Relates to #80167

…tic#80629) This allows specifying an optional level to be used for the deprecation handler registering a route. The default remains if the level is unspecified. Relates to elastic#80167

…tic#80629) This allows specifying an optional level to be used for the deprecation handler registering a route. The default remains if the level is unspecified. Relates to elastic#80167 # Conflicts: # server/src/main/java/org/elasticsearch/common/logging/DeprecationLogger.java # server/src/main/java/org/elasticsearch/rest/DeprecationRestHandler.java # server/src/main/java/org/elasticsearch/rest/RestController.java # server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestPutIndexTemplateAction.java # server/src/test/java/org/elasticsearch/rest/DeprecationRestHandlerTests.java # server/src/test/java/org/elasticsearch/rest/RestControllerTests.java

…) (#80776) This allows specifying an optional level to be used for the deprecation handler registering a route. The default remains if the level is unspecified. Relates to #80167

…) (#80779) This allows specifying an optional level to be used for the deprecation handler registering a route. The default remains if the level is unspecified. Relates to #80167 # Conflicts: # server/src/main/java/org/elasticsearch/common/logging/DeprecationLogger.java # server/src/main/java/org/elasticsearch/rest/DeprecationRestHandler.java # server/src/main/java/org/elasticsearch/rest/RestController.java # server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestPutIndexTemplateAction.java # server/src/test/java/org/elasticsearch/rest/DeprecationRestHandlerTests.java # server/src/test/java/org/elasticsearch/rest/RestControllerTests.java

jakelandis added 2 commits November 1, 2021 17:29

Change default level for deprecation logging

4ed1b5f

minor fixes

032d8a9

jakelandis added >non-issue v8.0.0 v8.1.0 labels Nov 1, 2021

jakelandis requested review from dakrone, masseyke and pgomulka November 1, 2021 22:35

pgomulka approved these changes Nov 2, 2021

View reviewed changes

jakelandis added 2 commits November 2, 2021 08:20

spotless

fdfb361

also drop non-settings down to warn

967264e

pgomulka approved these changes Nov 2, 2021

View reviewed changes

adjust tests to ensure compat warning and explicit headers are still …

09c1ca2

…critical

jakelandis commented Nov 2, 2021

View reviewed changes

jakelandis added 3 commits November 2, 2021 10:11

more tests for rest compat -> critical warning

a98db9c

more test fixes

cc1b22d

spotless

4f02c90

jakelandis commented Nov 3, 2021

View reviewed changes

elasticmachine and others added 2 commits November 3, 2021 10:59

Merge branch 'master' into change_default_level

efa8aad

another compat test fix and adjust transform for the correct level

6cf59eb

jakelandis added 5 commits November 3, 2021 14:44

more test fixes

4da4d55

more test fixes

29f59e8

more test fixes

4cd8d47

spotless

204b5b5

fix test and typo

b169d75

Merge branch 'master' into change_default_level

28f9658

dakrone approved these changes Nov 4, 2021

View reviewed changes

jakelandis added the auto-backport Automatically create backport pull requests when merged label Nov 4, 2021

jakelandis merged commit 255bf50 into elastic:master Nov 4, 2021

jakelandis deleted the change_default_level branch November 4, 2021 17:35

jakelandis removed the auto-backport Automatically create backport pull requests when merged label Nov 4, 2021

jakelandis mentioned this pull request Nov 4, 2021

[8.0] Change default level for deprecation logging (#80167) #80365

Merged

jakelandis mentioned this pull request Nov 4, 2021

[7.16] Drop non-setting deprecations down to warning level #80374

Merged

dakrone added a commit to dakrone/elasticsearch that referenced this pull request Nov 10, 2021

Allow specifying deprecation level for REST deprecation handler

bc81af3

This allows specifying an optional level to be used for the deprecation handler registering a route. Relates to elastic#80167

dakrone mentioned this pull request Nov 10, 2021

Allow specifying deprecation level for REST deprecation handler #80629

Merged

mark-vieira added v8.0.0-rc1 and removed v8.0.0 labels Jan 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change default level for deprecation logging#80167

Change default level for deprecation logging#80167
jakelandis merged 16 commits intoelastic:masterfrom
jakelandis:change_default_level

jakelandis commented Nov 1, 2021 •

edited

Loading

Uh oh!

pgomulka left a comment

Uh oh!

jakelandis commented Nov 2, 2021 •

edited

Loading

Uh oh!

pgomulka left a comment

Uh oh!

jakelandis Nov 2, 2021

Uh oh!

pgomulka Nov 2, 2021

Uh oh!

jakelandis Nov 2, 2021 •

edited

Loading

Uh oh!

jakelandis Nov 3, 2021

Uh oh!

jakelandis commented Nov 3, 2021

Uh oh!

masseyke commented Nov 3, 2021

Uh oh!

jakelandis commented Nov 4, 2021

Uh oh!

jakelandis commented Nov 4, 2021

Uh oh!

dakrone left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

jakelandis commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pgomulka left a comment

Choose a reason for hiding this comment

Uh oh!

jakelandis commented Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pgomulka left a comment

Choose a reason for hiding this comment

Uh oh!

jakelandis Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

pgomulka Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

jakelandis Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelandis Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

jakelandis commented Nov 3, 2021

Uh oh!

masseyke commented Nov 3, 2021

Uh oh!

jakelandis commented Nov 4, 2021

Uh oh!

jakelandis commented Nov 4, 2021

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jakelandis commented Nov 1, 2021 •

edited

Loading

jakelandis commented Nov 2, 2021 •

edited

Loading

jakelandis Nov 2, 2021 •

edited

Loading