-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Logging: Drop Settings from security logger get calls #33940
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
Conversation
`Settings` is no longer required to get a `Logger` and we went to quite a bit of effort to pass it to the `Logger` getters. This removes the `Settings` from all of the logger fetches in security and x-pack:core.
Pinging @elastic/es-core-infra |
This must have fallen into some CI downtime, so no jenkins run was triggered yet, so: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nik9000 I left a tiny question but in general LGTM
public ChainTransformFactory(Settings settings, TransformRegistry registry) { | ||
super(Loggers.getLogger(ExecutableChainTransform.class, settings)); | ||
public ChainTransformFactory(TransformRegistry registry) { | ||
super(Loggers.getLogger(ExecutableChainTransform.class)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity: in other instances you replaced Loggers.getLogger
with LogManager.getLogger
. Any advantaged of the later? If so, could this also be done here or doesn't it matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have done it here, yeah. The advantage of the latter is that the former is deprecated. It mostly just delegates to the latter.
@elasticmachine retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
`Settings` is no longer required to get a `Logger` and we went to quite a bit of effort to pass it to the `Logger` getters. This removes the `Settings` from all of the logger fetches in security and x-pack:core.
* master: Use more precise does S3 bucket exist method (elastic#34123) LLREST: Introduce a strict mode (elastic#33708) [CCR] Adjust list retryable errors (elastic#33985) Fix AggregationFactories.Builder equality and hash regarding order (elastic#34005) MINOR: Remove some deadcode in NodeEnv and Related (elastic#34133) Rest-Api-Spec: Correct spelling in filter_path description (elastic#33154) Core: Don't rely on java time for epoch seconds formatting (elastic#34086) Retry errors when fetching follower global checkpoint. (elastic#34019) Watcher: Reenable watcher stats REST tests (elastic#34107) Remove special-casing of Synonym filters in AnalysisRegistry (elastic#34034) Rename CCR APIs (elastic#34027) Fixed CCR stats api serialization issues and (elastic#33983) Support 'string'-style queries on metadata fields when reasonable. (elastic#34089) Logging: Drop Settings from security logger get calls (elastic#33940) SQL: Internal refactoring of operators as functions (elastic#34097)
`Settings` is no longer required to get a `Logger` and we went to quite a bit of effort to pass it to the `Logger` getters. This removes the `Settings` from all of the logger fetches in security and x-pack:core.
Settings
is no longer required to get aLogger
and we went to quitea bit of effort to pass it to the
Logger
getters. This removes theSettings
logger lookups from the security and x-pack:core.