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

Write deprecation logs to a data stream #58924

Closed

Conversation

pugnascotia
Copy link
Contributor

@pugnascotia pugnascotia commented Jul 2, 2020

Closes #46106. Introduce a mechanism for writing deprecation logs to a data stream as well as to disk.

Test by running ./gradlew run and then:

curl -u elastic-admin:elastic-password http://localhost:9200/_flush/synced?pretty
curl -u elastic-admin:elastic-password http://localhost:9200/logs-deprecation-elasticsearch/_search?pretty

This implementation reworks deprecation logging to rely on log4j by introducing filters and appenders, and a custom log level. The existing deprecation X-Pack plugin simply configures an extra appender to capture messages to an index. A "component" class manages most of the interface between the cluster and the appender.

This approach can be extended for other types of logging, e.g. security by introducing further custom log levels.

@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jul 2, 2020
@pugnascotia
Copy link
Contributor Author

I should add, the set of indexed fields in this PR may need to evolve, as it becomes clearer what we need to support the Upgrade Assistant. It should be a reasonable starting point however.


private static final String DEPRECATION_ORIGIN = "deprecation";

public static final Setting<Boolean> WRITE_DEPRECATION_LOGS_TO_INDEX = Setting.boolSetting(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the setting allow to flex between writing to a log, an index, or both ?

@pugnascotia pugnascotia marked this pull request as draft July 2, 2020 20:12
@pugnascotia
Copy link
Contributor Author

Moving to draft status while we reconsider some things.

@pugnascotia
Copy link
Contributor Author

@elasticmachine update branch

Copy link
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

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

it looks really good now. Thank you for working on this.
I left two minor comments.

import org.apache.logging.log4j.core.config.plugins.PluginFactory;
import org.apache.logging.log4j.message.Message;

@Plugin(name = "HeaderWarningAppender", category = Core.CATEGORY_NAME, elementType = Appender.ELEMENT_TYPE)
Copy link
Contributor

Choose a reason for hiding this comment

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

@pugnascotia any views on creating a logger?

@pugnascotia
Copy link
Contributor Author

@elasticmachine update branch

@pugnascotia
Copy link
Contributor Author

@rjernst @jakelandis CI is now green, can you please take another look?

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

I left a couple comments, but overall, would it be possible to split this up? The change pretty large, and doing several things, which I think are semi unrelated concerns:

  • Using log4j as backend of deprecation logs
  • Using log4j as backend of header warnings
  • Adding deprecation log messages from log4j to a data stream

@@ -9,6 +8,15 @@ esplugin {
}
archivesBaseName = 'x-pack-deprecation'

// add all sub-projects of the qa sub-project
gradle.projectsEvaluated {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

@@ -0,0 +1,8 @@
appender.header_warning.type = HeaderWarningAppender
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why this file is needed?

import org.apache.logging.log4j.core.config.plugins.PluginFactory;
import org.apache.logging.log4j.message.Message;

@Plugin(name = "HeaderWarningAppender", category = Core.CATEGORY_NAME, elementType = Appender.ELEMENT_TYPE)
Copy link
Member

Choose a reason for hiding this comment

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

I'm failing to understand what the advantage is of using log4j for header warnings? In either case, the warnings are stored in the thread context still right? Is it for formatting?

If we are going to use it, then I think it should be hidden, like deprecation logging is behind DeprecationLogger. But with header warnings, we already have the HeaderWarning class, and the logger retrieved will be the same for all locations, so we could just do this statically? Using a logger for header warnings directly is odd because the log level doesn't actual matter?

@pugnascotia
Copy link
Contributor Author

pugnascotia commented Aug 24, 2020

I'm breaking up the PR as follows:

Regarding header warnings: although I thought carefully before reimplementing header warnings with log4j...now I'm not so sure. That part of the work was relatively straightforward, so now I'm thinking that we only go down that road if/when we think we really need to.

@pugnascotia
Copy link
Contributor Author

Closing in favour of #61474 and #61484.

pugnascotia added a commit that referenced this pull request Sep 9, 2020
Backport of #58924.

Closes #46106. Introduce a mechanism for writing deprecation logs to a data stream
as well as to disk.
pugnascotia added a commit that referenced this pull request Nov 2, 2020
The implementation for indexing deprecation logs to a data stream
(#58924) relied on the Stack template for `logs-*-*`. This meant
that if the user disabled the stack templates, the templates would
also be unavailable for the deprecation logs.

Change the implementation so that:

* There is a separate template for deprecation logging
* The data stream is marked as hidden * The data stream name is
  prefixed with a period (`.`)
pugnascotia added a commit to pugnascotia/elasticsearch that referenced this pull request Nov 5, 2020
The implementation for indexing deprecation logs to a data stream
(elastic#58924) relied on the Stack template for `logs-*-*`. This meant
that if the user disabled the stack templates, the templates would
also be unavailable for the deprecation logs.

Change the implementation so that:

* There is a separate template for deprecation logging
* The data stream is marked as hidden * The data stream name is
  prefixed with a period (`.`)
pugnascotia added a commit that referenced this pull request Nov 6, 2020
Backport of #64417. 

The implementation for indexing deprecation logs to a data stream (#58924)
relied on the Stack template for `logs-*-*`. This meant that if the user
disabled the stack templates, the templates would also be unavailable for
the deprecation logs.

Change the implementation so that:

* There is a separate template for deprecation logging
* The data stream is marked as hidden
* The data stream name is prefixed with a period (`.`)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Logging Log management and logging utilities >enhancement Team:Core/Infra Meta label for core/infra team v7.10.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write deprecation logs to an index
6 participants