Skip to content

Conversation

@Jongy
Copy link
Contributor

@Jongy Jongy commented Jan 9, 2022

Description

When profiling of a process is skipped by gProfiler for whatever reason, the following happens:

  • In perf-less mode (--perf-mode=none) - we won't see its stacks and we won't know it existed.
  • In standard mode, gProfiler will use the samples collected by perf (if any) when merging the stacks. So we'll see the native Java code, which is seldom interesting.

This PR amends the Java profiler to return a specialized stack per "skip" case (safemode, AP already loaded in a process, ...). This way, the user sees the cause of the error immediately, and doesn't have to skim through the logs in order to understand. Also, it makes the skip explicit ("gProfiler found this process and decided to skip", instead of "maybe gProfiler missed it..."). Explicitness is good.

Motivation and Context

Make it easier to understand when & why gProfiler decides to skip Java profiling.

How Has This Been Tested?

  • Profiling a process with AP loaded (from a different path) - results in java;[Profiling skipped: async-profiler is already loaded].
  • Profiling a process with unsupported JVM version / type - results in java;[Profiling skipped: profiling this JVM is not supported].
  • Profiling a process after safemode has disabled profiling completely - results in the profiling skipped stack, with the safemode reason in it. For example: java;[Profiling skipped: disabled due to hserr]

Automatic tests for all the above ^^

@Jongy Jongy requested a review from DanielShaulov January 16, 2022 23:24
@Jongy Jongy force-pushed the java-error-stack-when-profiling-is-skipped branch from 82b8084 to aa13fdb Compare January 16, 2022 23:25
Copy link

@DanielShaulov DanielShaulov left a comment

Choose a reason for hiding this comment

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

Minor comments, looks good.
I think you are also missing a test for bad/incompatible java version, but I understand if it is too hard to do and not worth it

@Jongy Jongy requested a review from DanielShaulov January 17, 2022 11:12
@Jongy
Copy link
Contributor Author

Jongy commented Jan 17, 2022

Minor comments, looks good.
I think you are also missing a test for bad/incompatible java version, but I understand if it is too hard to do and not worth it

We have test_java_safemode_version_check and test_java_safemode_build_number_check, but no test for unsupported JVM type because it's indeed hard to implement.

@Jongy Jongy merged commit e58352c into master Jan 17, 2022
@Jongy Jongy deleted the java-error-stack-when-profiling-is-skipped branch January 17, 2022 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request runtime/java

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants