Skip to content

Remove stacktraces for RE failures on #18338

Merged
cryptoe merged 2 commits intoapache:masterfrom
uds5501:log_with_no_stacktrace
Jul 31, 2025
Merged

Remove stacktraces for RE failures on #18338
cryptoe merged 2 commits intoapache:masterfrom
uds5501:log_with_no_stacktrace

Conversation

@uds5501
Copy link
Contributor

@uds5501 uds5501 commented Jul 29, 2025

Removes stack traces for RE exception thrown when calling discover() on a misconfigured cpu discoverer

}
catch (IOException | RuntimeException ex) {
LOG.error(ex, "Unable to fetch cpu snapshot");
LOG.noStackTrace().error(ex, "Unable to fetch cpu snapshot");
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be warn. Error is reserved for serious errors, but this isn't one, it just means that some metrics won't be reported. Btw, the error message should have more detail about the impact. It should mention that this means that cgroup metrics will not be emitted. Such as:

Unable to fetch CPU snapshot. Cgroup metrics will not be emitted.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the error message can also mention that try using the CgroupV2CpuMonitor ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if it is possible to detect which one is present and switch into the right one automatically? That seems better than trying to write an error message that guides people to make the right config change.

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 don't think that would work, within CgroupV2CpuMonitor is supposed to use ProcCgroupV2Discoverer , that itself does not seem to implement the discover(String cgroup) completely . The cgroup is not being used in the function anywhere so I am not confident that it's complete in itself.

Additionally CgroupV2CpuMonitor is marked experimental so I don't think we should be switching it internally.

@uds5501
Copy link
Contributor Author

uds5501 commented Jul 29, 2025

Thanks for the review @gianm , I have accommodated the updates.

@uds5501 uds5501 requested review from cryptoe and gianm July 30, 2025 17:09
@cryptoe cryptoe merged commit 75b9916 into apache:master Jul 31, 2025
71 of 72 checks passed
@cecemei cecemei added this to the 35.0.0 milestone Oct 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants