Change default deprecation logger level to CRITICAL#77030
Change default deprecation logger level to CRITICAL#77030pgomulka merged 14 commits intoelastic:masterfrom
Conversation
| * The message is also sent to the header warning logger, | ||
| * so that it can be returned to the client. | ||
| */ | ||
| public DeprecationLogger deprecateAtWarnLevel( |
There was a problem hiding this comment.
hmm maybe deprecateNonCritical is better?
There was a problem hiding this comment.
Hmm...I wonder about perhaps just using a log4j style e.g.:
deprecationLogger.critical(DeprecationCategory.OTHER, "broken in next version");
deprecationLogger.warn(DeprecationCategory.OTHER, "only somewhat broken in next version");There was a problem hiding this comment.
I like the idea overall, but I wonder if this would not make it a bit too clear for developers to know which method to use?
I feel like people are used to deprecationLogger.deprecate which helps them to forget about log4j level detail.
Any thoughts on this?
There was a problem hiding this comment.
I actually like the idea of developers being required to think about their deprecation messages more carefully, because these messages will be exposed far more directly to customers in the future. If we used critical() and warn(), we could circulate an email about es@ about it, and maybe update the contributing guide.
|
@elasticmachine update branch |
…icsearch into logging/deprecation_level
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
server/src/main/java/org/elasticsearch/common/logging/DeprecationLogger.java
Outdated
Show resolved
Hide resolved
|
|
||
| /** | ||
| * Same as <code>DeprecationIndexingAppender#DEPRECATION_MESSAGES_DATA_STREAM</code>, but that class isn't visible from here. | ||
| * Same as <code>DeprecationIndexingAppender#CRITICAL_MESSAGES_DATA_STREAM</code>, but that class isn't visible from here. |
There was a problem hiding this comment.
Is this an accidental change?
There was a problem hiding this comment.
ah yes.. looks like accidentally intellij refactored this for me too..
| hasEntry(KEY_FIELD_NAME, "deprecated_settings"), | ||
| hasEntry("event.dataset", "deprecation.elasticsearch"), | ||
| hasEntry("log.level", "DEPRECATION"), | ||
| hasEntry("log.level", "CRITICAL"), |
There was a problem hiding this comment.
It would be good if we could also trigger a WARN level deprecation and observe that it is written to the data stream.
Co-authored-by: Rory Hunter <pugnascotia@users.noreply.github.com>
pugnascotia
left a comment
There was a problem hiding this comment.
LGTM. I'm not keen on compatibleCritical but I can't think of anything better.
|
@elasticmachine update branch |
This commit changes default deprecation logger level to CRITICAL, where default means deprecations emitted by DeprecationLogger#critical method. It also introduces WARN deprecations which are emitted by DeprecationLogger#warn Those log lines emitted at WARN are meant to indicate that a functionality is deprecated but will not break at next major version. relates elastic#76754
|
@pgomulka - Does the documentation at https://www.elastic.co/guide/en/elasticsearch/reference/7.x/logging.html#deprecation-logging need to also be updated ? |
…77482) This commit changes default deprecation logger level to CRITICAL, where default means deprecations emitted by DeprecationLogger#critical method. It also introduces WARN deprecations which are emitted by DeprecationLogger#warn Those log lines emitted at WARN are meant to indicate that a functionality is deprecated but will not break at next major version. relates #76754
adding a section on WARN messages relates elastic#77030
|
@jakelandis some documentation was added in https://github.com/elastic/elasticsearch/pull/77030/files#diff-db0a51f5867b2c602fa7e8c06259098845ece293e06d75763416fb0dbb0f49f9 but in fact, I think some more information about WARN level should be added too. |
adding a section on WARN messages relates #77030
adding a section on WARN messages relates elastic#77030
This commit changes default deprecation logger level to CRITICAL, where default means deprecations emitted by
DeprecationLogger#criticalmethod.It also introduces WARN deprecations which are emitted by
DeprecationLogger#warnThose log lines emitted at WARN are meant to indicate that a functionality is deprecated but will not break at next major version.relates #76754