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

Replace stack walking getLogger with explicit calls #84480

Merged
merged 4 commits into from
Mar 2, 2022

Conversation

ChrisHegarty
Copy link
Contributor

Replace the no-args LogManager::getLogger calls with the single-arg
variant that accepts a j.l.Class reference, which avoids the stack walk
of the no-args variant. The no-args variant determines the caller's
class by looking at the stack frame two positions from itself. The use
of the 1-args variant is more explicit and avoids the need for the stack
walk, while retaining the very same behaviour. Standardizing on the
1-args variant will reduce the need to have different ways to retrieve
logger references.

@ChrisHegarty ChrisHegarty requested a review from pgomulka March 1, 2022 09:41
@ChrisHegarty ChrisHegarty added the Team:Core/Infra Meta label for core/infra team label Mar 1, 2022
@elasticmachine
Copy link
Collaborator

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

@ChrisHegarty ChrisHegarty mentioned this pull request Mar 1, 2022
19 tasks
@DaveCTurner
Copy link
Contributor

👍 but we should also adjust the contribution docs which say to use the no-arg constructor: https://github.com/elastic/elasticsearch/blob/master/CONTRIBUTING.md#logging

Also consider adding the no-arg constructor to build-tools-internal/src/main/resources/forbidden/es-server-signatures.txt so that this doesn't get unconsciously and progressively reverted over time.

@pugnascotia pugnascotia added :Core/Infra/Logging Log management and logging utilities >refactoring and removed >non-issue labels Mar 1, 2022
@ChrisHegarty
Copy link
Contributor Author

Great suggestions @DaveCTurner

👍 but we should also adjust the contribution docs which say to use the no-arg constructor: https://github.com/elastic/elasticsearch/blob/master/CONTRIBUTING.md#logging

Done.

Also consider adding the no-arg constructor to build-tools-internal/src/main/resources/forbidden/es-server-signatures.txt so that this doesn't get unconsciously and progressively reverted over time.

Done.

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.

LGTM

@ChrisHegarty ChrisHegarty merged commit ae8eeae into elastic:master Mar 2, 2022
@ChrisHegarty ChrisHegarty deleted the stack_logger branch March 2, 2022 09:42
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 >refactoring Team:Core/Infra Meta label for core/infra team v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants