Skip to content

Do not stop profiling on fatal exceptions #1421

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.

Sign up for GitHub

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

Merged
mar-kolya merged 2 commits into from
May 4, 2020
Merged

Do not stop profiling on fatal exceptions #1421

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
014cb92
Do not stop profiling on fatal exceptions
mar-kolya May 1, 2020
3295f19
Catch and ignore exceptions in logging
mar-kolya May 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions .../profiling-controller/src/main/java/com/datadog/profiling/controller/ProfilingSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,8 +199,18 @@ public void run() {
} catch (final Exception e) {
log.error("Exception in profiling thread, continuing", e);
} catch (final Throwable t) {
log.error("Fatal exception in profiling thread, exiting", t);
throw t;
/*
Try to continue even after fatal exception. It seems to be useful to attempt to store profile when this happens.
For example JVM maybe out of heap and throwing OutOfMemoryError - we probably still would want to continue and
Copy link
Copy Markdown
Contributor

@jbachorik jbachorik May 4, 2020

Choose a reason for hiding this comment

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

For OOME usually it is not really good to try and and log exception - the memory is stretched already and the logging statement can throw its own OOME and propagate the exception we are trying to mask to the caller.

I would suggest handling OOME differently and just silently ignore it.

Copy link
Copy Markdown
Contributor Author

@mar-kolya mar-kolya May 4, 2020

Choose a reason for hiding this comment

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

good point, I'll just wrap the whole logging statement in try/catch

Copy link
Copy Markdown
Contributor

@dbenamydd dbenamydd May 4, 2020

Choose a reason for hiding this comment

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

Good idea to try the log if possible!

try to save profile later.
Another reason is that it may be bad to stop profiling if the rest of the app is continuing.
*/
try {
log.error("Fatal exception in profiling thread, trying to continue", t);
} catch (final Throwable t2) {
// This should almost never happen and there is not much we can do here in cases like
// OutOfMemoryError, so we will just ignore this.
}
}
}
}
Expand Down