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

Introduce deprecation categories #68061

Merged
merged 6 commits into from
Jan 29, 2021

Conversation

pugnascotia
Copy link
Contributor

Sort-of backport of #67443.

Closes #64824. Introduce the concept of categories to deprecation
logging. Every location where we log a deprecation message must now
include a deprecation category.

pugnascotia and others added 4 commits January 25, 2021 12:24
The JSON logs that Elasticsearch produces are roughly in an ECS shape. This PR improves
that alignment.
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.
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jan 27, 2021
@elasticmachine
Copy link
Collaborator

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

@williamrandolph williamrandolph self-requested a review January 27, 2021 19:58
Copy link
Contributor

@williamrandolph williamrandolph left a comment

Choose a reason for hiding this comment

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

Whew, that's a lot of deprecations. I had a few idle thoughts but nothing blocking, so LGTM.

}
if (BootstrapInfo.getSystemProperties().get("es.xcontent.strict_duplicate_detection") != null) {
final String message = String.format(
Locale.ROOT,
"The Java option es.xcontent.strict_duplicate_detection is set to [%s]; " +
"this option is deprecated and non-functional and should be removed from Java configuration.",
BootstrapInfo.getSystemProperties().get("es.xcontent.strict_duplicate_detection"));
DeprecationLogger.getLogger(Bootstrap.class).deprecate("strict_duplicate_detection_setting_removed", message);
DeprecationLogger.getLogger(Bootstrap.class).deprecate(DeprecationCategory.OTHER, "strict_duplicate_detection_setting_removed",
Copy link
Contributor

Choose a reason for hiding this comment

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

Arguably this could be SETTINGS, although it's a JVM setting and not an Elasticsearch setting.

@@ -277,7 +278,8 @@ private Long parseNullValue(DateFieldType fieldType) {
try {
return fieldType.parse(nullValue.getValue());
} catch (Exception e) {
DEPRECATION_LOGGER.deprecate("date_mapper_null_field", "Error parsing [" + nullValue.getValue()
DEPRECATION_LOGGER.deprecate(DeprecationCategory.MAPPINGS, "date_mapper_null_field",
"Error parsing [" + nullValue.getValue()
Copy link
Contributor

Choose a reason for hiding this comment

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

extreme, probably-shouldn't-even-write-this-comment nitpick: putting the line break after field [" might be slightly more visually appealing

(I hope these tiny nitpicks give you confidence in the thoroughness of my review, rather than just annoying you.)

AccessController.doPrivileged(new PrivilegedAction<Void>() {
@SuppressLoggerChecks(reason = "safely delegates to logger")
@Override
public Void run() {
deprecationLogger.deprecate(key, message, params);
deprecationLogger.deprecate(category, key, message, params);
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider just passing in DeprecationCategory.PARSING here instead of altering the methods below. Since this method is private I think it's safe to say it'll be used for parsing issues within this class.

@pugnascotia
Copy link
Contributor Author

Thank you @williamrandolph!

@pugnascotia pugnascotia merged commit 1c5b89c into elastic:7.x Jan 29, 2021
@pugnascotia pugnascotia deleted the 64824-deprecation-category-7x branch January 29, 2021 14:31
pgomulka added a commit that referenced this pull request Oct 18, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport :Core/Infra/Logging Log management and logging utilities >enhancement Team:Core/Infra Meta label for core/infra team v7.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants