Avoid logging stack trace during expected remote-config exception#11328
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd76457b77
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } catch (Exception e) { | ||
| // no error can be reported, as we don't have the data client.state.targets_version avail | ||
| ratelimitedLogger.warn("Error parsing remote config response", e); | ||
| ratelimitedLogger.warn("Error parsing remote config response: {}", e.toString()); |
There was a problem hiding this comment.
e.getMessage() might be better.
I'm also not sure if we still want to pass the exception somehow for exception metric reporting purposes. I've always thought we should have a helper that handles this better.
Something like warnExpected / warnQuielty?. Although, I guess use of a rateLimitedLogger is itself an indication that an exception is expected here.
There was a problem hiding this comment.
Also worth thinking about whether, all Exceptions are expecting or only a subset.
There was a problem hiding this comment.
Sounds good. It looks like ClosedSelectorExceptions are what's expected when selectors are closed mid-poll, so I'll update those exceptions specifically to use e.getMessage().
sarahchen6
force-pushed
the
sarahchen6/apms-19495
branch
from
b22d2bc to
0e6bdee
Compare
PerfectSlayer
left a comment
PerfectSlayer
left a comment
There was a problem hiding this comment.
Left a minor comment
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
3 participants
What Does This Do
Avoid logging the full stack trace during an expected remote-config exception. Instead log just the exception class and message.
Motivation
This exception occurs when the tracer's selector get closed mid-poll which is often the case when the pod is shutdown. Since this exception is expected, avoid logging a full stack trace and unnecessarily alarming users.
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.