Skip to content

Conversation

mosche
Copy link
Contributor

@mosche mosche commented Jul 10, 2025

Log NotEntitledExceptions using logger with <component>.<package> suffix rather than <component>.<module> for more fine-grained mutes.

Relates to ES-12231

…fix rather than <component>.<module> for more fine-grained mutes.

Relates to ES-12231
@mosche mosche requested a review from a team July 10, 2025 16:00
@mosche mosche added >non-issue :Core/Infra/Entitlements Entitlements infrastructure labels Jul 10, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jul 10, 2025
@elasticsearchmachine
Copy link
Collaborator

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

}

private void notEntitled(String message, Class<?> callerClass, ModuleEntitlements entitlements) {
private void notEntitled(String message, Class<?> requestingClass, ModuleEntitlements entitlements) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this fixes a potential NPE: if callerClass != null, requestingClass is callerClass

Copy link
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

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

LGTM!
Just double check please if any of the existing mutes need updating to go with this change.

@mosche
Copy link
Contributor Author

mosche commented Jul 14, 2025

Ugh, you're right @ldematte ... and the current mutes have no comment as for which class failed previously, particularly hard with ALL-UNNAMED :/

Changed it to <component>.<module>.<package> so old mutes will continue working.
Unfortunately logger names might get really long that way, but this should happen only rarely

@prdoyle
Copy link
Contributor

prdoyle commented Jul 14, 2025

It's a bit unfortunate to be burdened with legacy already, so early in the lifecycle of this project, but I can live with this if Lorenzo likes it.

<component>.<module>.<package> is clever, though it is somewhat ambiguous because a logger called component.a.b.c could be module a and package b.c or it could be module a.b and package c.

If we wanted to go with <component>.<package>...

The initial set of muted warnings probably occur during the CI tests, so we could probably delete the mutes and look for warnings in the CI output to determine the packages for those. For mutes added later, a git blame could take us to the PR that added them, and hopefully the PR would record enough info that we could determine the package name.

@ldematte
Copy link
Contributor

If we want, could we use a different separator? And/or make <module> optional?
E.g. <component>.<module>#<package> and/or allow <component>#<package> (no module)?
Just an idea, I think I'm fine with <component>.<module>.<package> too :)

@mosche
Copy link
Contributor Author

mosche commented Jul 14, 2025

The initial set of muted warnings probably occur during the CI tests, so we could probably delete the mutes and look for warnings in the CI output to determine the packages for those. For mutes added later, a git blame could take us to the PR that added them, and hopefully the PR would record enough info that we could determine the package name.

I checked old PRs that added mutes, there's not much context documented there :/

If we want, could we use a different separator? And/or make optional?

Making <module> optional would also break previous mutes.
The separator has to be a dot I think, otherwise logger inheritance won't work

@mosche
Copy link
Contributor Author

mosche commented Jul 14, 2025

.. is clever, though it is somewhat ambiguous because a logger called component.a.b.c could be module a and package b.c or it could be module a.b and package c.

An alternative could be to have both <component>.<module> and <component>.<package> and prior to returning a logger for <component>.<package> check if <component>.<module> is muted, and if so return a no-op logger.
That way we'd support both module and package based mutes without overcomplicating the logger names for package based mutes.

It's just not vanilla log4j anymore that way and one has to remember there's two different ways of doing mutes.

@ldematte
Copy link
Contributor

The separator has to be a dot I think, otherwise logger inheritance won't work

Let's go with the current scheme then (<component>.<module>.<package>)

@mosche mosche added auto-backport Automatically create backport pull requests when merged v9.1.1 v8.19.1 labels Jul 14, 2025
@mosche mosche merged commit 92ab85d into elastic:main Jul 15, 2025
33 checks passed
mosche added a commit to mosche/elasticsearch that referenced this pull request Jul 15, 2025
…#131031)

Log NotEntitledExceptions using logger with `<component>.<module>.<package>` suffix (instead of `<component>.<module>`) for more fine-grained mutes, but remaining backwards compatible regarding existing mutes.

Relates to ES-12231
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
9.1
8.19

mosche added a commit to mosche/elasticsearch that referenced this pull request Jul 15, 2025
…#131031)

Log NotEntitledExceptions using logger with `<component>.<module>.<package>` suffix (instead of `<component>.<module>`) for more fine-grained mutes, but remaining backwards compatible regarding existing mutes.

Relates to ES-12231
elasticsearchmachine pushed a commit that referenced this pull request Jul 15, 2025
#131290)

Log NotEntitledExceptions using logger with `<component>.<module>.<package>` suffix (instead of `<component>.<module>`) for more fine-grained mutes, but remaining backwards compatible regarding existing mutes.

Relates to ES-12231
elasticsearchmachine pushed a commit that referenced this pull request Jul 15, 2025
#131289)

Log NotEntitledExceptions using logger with `<component>.<module>.<package>` suffix (instead of `<component>.<module>`) for more fine-grained mutes, but remaining backwards compatible regarding existing mutes.

Relates to ES-12231
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :Core/Infra/Entitlements Entitlements infrastructure >non-issue Team:Core/Infra Meta label for core/infra team v8.19.1 v9.1.1 v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants