Introduce deprecation categories #67443
Conversation
pugnascotia
added
>enhancement
:Core/Infra/Logging
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
jdconrad
left a comment
jdconrad
left a comment
There was a problem hiding this comment.
@pugnascotia I wonder if we should add more categories such as "plugin" or "parsing" since those would cover a lot of the "other" category if we consider most of the date deprecations as parsing issues.
pgomulka
left a comment
pgomulka
left a comment
There was a problem hiding this comment.
I left one comment, but it looks good to me
| */ | ||
| public DeprecationLoggerBuilder deprecate(final String key, final String msg, final Object... params) { | ||
| return new DeprecationLoggerBuilder().withDeprecation(key, msg, params); | ||
| public DeprecationLoggerBuilder deprecate( |
There was a problem hiding this comment.
btw do you think it would be worthy to make this method work in a "fluent" way?
I initialliy thought that deprecation logger could be used like
deprecationLogger.deprecate("msg")
.compatibleWarning("msg2")
The compatible logger is still not merged (going to be soon I hope), but maybe we could use the builder here?
I am worried about the performance and we won't be creating too much objects here though..
WDYT?
There was a problem hiding this comment.
I don't follow - deprecationLogger.deprecate(...) already returns a DeprecationLoggerBuilder, so isn't it already fluent?
| /** | ||
| * Deprecation log messages are categorised so that consumers of the logs can easily aggregate them. | ||
| */ | ||
| public enum DeprecationCategory { |
There was a problem hiding this comment.
would would it make any sense for this to be an interface and some of the categories to be defined in server and some in plugins/xpack?
We will be using deprecations a lot in x-pack/rest-compatible plugin and I wonder if we should be adding all the categories from plugins here?
There was a problem hiding this comment.
I'm not sure about doing that, as the categories ought to be reasonable general and not too fine-grained. We could change the implementation later if we feel that we do need more flexibility.
| .field("ecs.version", ECS_VERSION) | ||
| .field("key", key); | ||
| .field("key", key) | ||
| .field("elasticsearch.event.category", category.name().toLowerCase(Locale.ROOT)); |
There was a problem hiding this comment.
Why not just category? Is this ECS related?
Sort-of backport of elastic#67443. Closes elastic#64824. Introduce the concept of categories to deprecation logging. Every location where we log a deprecation message must now include a deprecation category.
Deprecation logs in 7.x are using ESJsonLayout and it requires additional fields to be declared in a log4j config in order to emit values to logs. This commit adds category field to deprecation log pattern context: in 8.0 we use ECSLayout which do not require to declare additional fields upfront. In 7.x we have to declare them in the logging config. The PR introducing categories in master #67443. A backport to 7.x which missed this field #68061
6 participants
Closes #64824. Introduce the concept of categories to deprecation logging. Every location where we log a deprecation message must now include a deprecation category.
See also #67266.
All opinions welcome on the categories I've used here. I'd be very happy to shrink the number here, but I also don't want the categories to be so broad as to be meaningless.
Note that I probably won't directly backport this - instead, I expect I'll have to reapply the
DeprecationMessagechange and then go through all the call sites again.