Skip to content

Conversation

@mosche
Copy link
Contributor

@mosche mosche commented Feb 19, 2025

Log message for troubleshooting if not entitled.

@mosche mosche requested a review from a team as a code owner February 19, 2025 15:32
@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v9.1.0 labels Feb 19, 2025
@mosche mosche added Team:Core/Infra Meta label for core/infra team auto-backport Automatically create backport pull requests when merged test-entitlements v8.18.1 v8.19.0 v9.0.1 :Core/Infra/Entitlements Entitlements infrastructure >non-issue and removed needs:triage Requires assignment of a team area label v9.1.0 labels Feb 19, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@mosche mosche added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Feb 19, 2025
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.

One suggestion

private static void notEntitled(String message, Class<?> callerClass) {
// don't log self tests in EntitlementBootstrap
if (EntitlementBootstrap.class.equals(callerClass) == false) {
logger.warn(message);
Copy link
Member

Choose a reason for hiding this comment

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

If you construct the not entitled exception first then the exception can be logged, along with the stack trace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about doing it, but decided against it as we'd be logging the exception twice most of the time. I often find that more confusing / disturbing than being helpful in rare cases. On the other hand, obviously, those warnings are a lot more noticeable if there's a stacktrace attached.
I don't have a strong preference, happy to follow the crowed here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel strongly in this case. The message is likely to have enough info to determine the remedy already.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same, not a strong preference. OK to leave it as-is -- like you said, this is mainly for when we have code that catches Throwable or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 added

@mosche mosche removed the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Feb 19, 2025
@mosche mosche added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Feb 19, 2025
@elasticsearchmachine elasticsearchmachine merged commit 2c15b68 into elastic:main Feb 19, 2025
22 checks passed
@mosche mosche deleted the entitlements/log-not-entitled branch February 19, 2025 19:14
mosche added a commit to mosche/elasticsearch that referenced this pull request Feb 19, 2025
mosche added a commit to mosche/elasticsearch that referenced this pull request Feb 19, 2025
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.18
8.x
9.0

mosche added a commit to mosche/elasticsearch that referenced this pull request Feb 19, 2025
elasticsearchmachine pushed a commit that referenced this pull request Feb 19, 2025
…) (#122961)

Log message for troubleshooting if not entitled.
elasticsearchmachine pushed a commit that referenced this pull request Feb 19, 2025
…) (#122963)

Log message for troubleshooting if not entitled.
elasticsearchmachine pushed a commit that referenced this pull request Feb 19, 2025
…) (#122962)

Log message for troubleshooting if not entitled.
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 auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Core/Infra/Entitlements Entitlements infrastructure >non-issue Team:Core/Infra Meta label for core/infra team v8.18.1 v8.19.0 v9.0.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants