Add start-up diagnostics by jeanbisutti · Pull Request #2500 · microsoft/ApplicationInsights-Java

jeanbisutti · 2022-09-06T14:45:10Z

Add start-up diagnostics:

Disable C2 compiler during start-up
JVM flags (removed from this PR)
Resident-set size
Native memory tracking

…ights/agent/internal/init/StartupDiagnostics.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

trask · 2022-09-12T20:47:32Z

+      "applicationinsights.debug.rss.enabled";
+
+  // Execute with -XX:NativeMemoryTracking=summary
+  private static final String APPLICATIONINSIGHTS_DEBUG_NATIVE_MEM_TRACKING_ENABLED =
+      "applicationinsights.debug.native-mem-tracking.enabled";
+
+  public static final String APPLICATIONINSIGHTS_DEBUG_DIAG_EXPORT_TO_FILE =
+      "applicationinsights.debug.diag-export-to-file";


maybe applicationinsights.debug.* -> applicationinsights.debug.startup.*, or applicationinsights.debug.startup-*?

trask · 2022-09-12T20:52:11Z

+    byte[] diagReportAsBytes = diagnosticsReport.toString().getBytes(StandardCharsets.UTF_8);
+    try {
+      Files.write(diagFile.toPath(), diagReportAsBytes);


Suggested change

byte[] diagReportAsBytes = diagnosticsReport.toString().getBytes(StandardCharsets.UTF_8);

try {

Files.write(diagFile.toPath(), diagReportAsBytes);

try {

Files.writeString(diagFile.toPath(), diagnosticsReport.toString());

@trask Files.writeString is not availlable in Java 8: https://docs.oracle.com/javase/8/docs/api/java/nio/file/Files.html

trask · 2022-09-12T20:58:24Z

+    }
+  }
+
+  private Optional<File> createTempDirIfNotExists() {


typically this repo (and upstream otel) uses @Nullable over Optional, though not totally consistently, it's not a problem to introduce here, but let's have broader discussion separately over whether we want to go in that direction generally

trask · 2022-09-12T21:05:40Z

+  }
+
+  private static IllegalStateException combineExceptionsIfNecessary(
+      IllegalStateException exitValueException, Exception e, List<String> command) {


Suggested change

IllegalStateException exitValueException, Exception e, List<String> command) {

@Nullable IllegalStateException exitValueException, Exception e, List<String> command) {

trask · 2022-09-12T21:08:26Z

+      if (e.getSuppressed().length == 1) {
+        return e.getMessage() + " (Suppressed: " + e.getSuppressed()[0] + ")";
+      }


The suppressed message doesn't seem like it would be very interesting typically, and this would allow still getting the full stack trace along with suppressed exception if really needed at debug level.

Suggested change

if (e.getSuppressed().length == 1) {

return e.getMessage() + " (Suppressed: " + e.getSuppressed()[0] + ")";

}

startupLogger.debug(e.getMessage(), e);

heyams · 2022-09-12T21:39:25Z

+  // ]
+  private static final String
+      APPLICATIONINSIGHTS_EXPERIMENT_CLEAR_COMPILER_DIRECTIVES_AFTER_INITIALIZATION =
+          "applicationinsights.experiment.clear-compiler-directives-after-initialization";


Suggested change

"applicationinsights.experiment.clear-compiler-directives-after-initialization";

"applicationinsights.experiment.clear_compiler_directives_after_initialization";

should we follow the same naming convention? i have seen lost of underline instead of dash. See AiSemanticAttributes.java

this is a system property name, and not a semantic attribute name

heyams · 2022-09-12T21:41:02Z

+
+  // Execute with -XX:NativeMemoryTracking=summary
+  private static final String APPLICATIONINSIGHTS_DEBUG_NATIVE_MEM_TRACKING_ENABLED =
+      "applicationinsights.debug.native-mem-tracking.enabled";


Suggested change

"applicationinsights.debug.native-mem-tracking.enabled";

"applicationinsights.debug.native_mem_tracking_enabled";

heyams · 2022-09-12T21:49:19Z

+
+  static boolean hasToDisableJvmCompilerDirectives() {
+    return Boolean.getBoolean(
+        APPLICATIONINSIGHTS_EXPERIMENT_CLEAR_COMPILER_DIRECTIVES_AFTER_INITIALIZATION);


such a long system property name, should we consider a shorter name?

this is a hard one to explain succinctly 😅

heyams · 2022-09-12T21:49:41Z

+
+class JvmCompiler {
+
+  // To disable C2 compiler during Application Insights set-up, for Java >= 9, do the following


can you add a note how user can use APPLICATIONINSIGHTS_EXPERIMENT_CLEAR_COMPILER_DIRECTIVES_AFTER_INITIALIZATION?

@heyamsthis It's an experimental feature that we would like to test with some users. Following the feedbacks, we could properly document this feature.

@heyams It is necessary to add the JVM option explained from line 11 and set the applicationinsights.experiment.clear-compiler-directives-after-initialization property to true.

…ights/agent/internal/init/StartupDiagnostics.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

…and the ability to disable the JVM C2 compiler during start-up (#2500)

Add start-up diagnostics

32b3bce

jeanbisutti requested review from heyams and trask as code owners September 6, 2022 14:45

jeanbisutti and others added 3 commits September 6, 2022 07:50

Fix error prone

070f50b

Error prone

d5d0463

Make security check happy

a251727

trask reviewed Sep 9, 2022

View reviewed changes

jeanbisutti added 7 commits September 9, 2022 02:53

Remove JVM flags from diagnostics

d1a2c94

Rename RSS property

6ecfb95

Method renamed

1a882cd

Apply spotless

2e9e789

Add new class (SRP)

5165fbb

Fix typos

d29a146

Default export is now console

d11d588

trask reviewed Sep 9, 2022

View reviewed changes

Comment thread .../src/main/java/com/microsoft/applicationinsights/agent/internal/init/StartupDiagnostics.java Outdated

jeanbisutti and others added 2 commits September 9, 2022 14:18

Update agent/agent-tooling/src/main/java/com/microsoft/applicationins…

8be0cff

…ights/agent/internal/init/StartupDiagnostics.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Fix compilation

ca04101

jeanbisutti requested a review from trask September 9, 2022 21:50

trask approved these changes Sep 12, 2022

View reviewed changes

heyams reviewed Sep 12, 2022

View reviewed changes

jeanbisutti and others added 9 commits September 13, 2022 10:41

Update agent/agent-tooling/src/main/java/com/microsoft/applicationins…

2d14e3f

…ights/agent/internal/init/StartupDiagnostics.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Update agent/agent-tooling/src/main/java/com/microsoft/applicationins…

b23a115

…ights/agent/internal/init/StartupDiagnostics.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Add "debug" in property names

cedc90c

Update agent/agent-tooling/src/main/java/com/microsoft/applicationins…

8b39576

…ights/agent/internal/init/StartupDiagnostics.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Remove log injection in StartupDiagnostics

30f5a4b

Add @nullable

eceb72e

Fix error prone

fba1df7

Fix error prone

2210340

Fix error prone

79c859c

From object to static

20a9d1b

jeanbisutti merged commit b95f119 into main Sep 13, 2022

jeanbisutti deleted the diags branch September 13, 2022 10:10

jeanbisutti restored the diags branch September 13, 2022 10:27

heyams pushed a commit that referenced this pull request Sep 16, 2022

Add start-up diagnostics (resident-set size, native memory tracking) …

0581689

…and the ability to disable the JVM C2 compiler during start-up (#2500)

heyams pushed a commit that referenced this pull request Sep 16, 2022

Add start-up diagnostics (resident-set size, native memory tracking) …

75a336c

…and the ability to disable the JVM C2 compiler during start-up (#2500)

heyams pushed a commit that referenced this pull request Sep 16, 2022

Add start-up diagnostics (resident-set size, native memory tracking) …

2d85732

…and the ability to disable the JVM C2 compiler during start-up (#2500)

trask deleted the diags branch September 28, 2022 21:40

	IllegalStateException exitValueException, Exception e, List<String> command) {
	@Nullable IllegalStateException exitValueException, Exception e, List<String> command) {

	"applicationinsights.experiment.clear-compiler-directives-after-initialization";
	"applicationinsights.experiment.clear_compiler_directives_after_initialization";

	"applicationinsights.debug.native-mem-tracking.enabled";
	"applicationinsights.debug.native_mem_tracking_enabled";


		class JvmCompiler {

		// To disable C2 compiler during Application Insights set-up, for Java >= 9, do the following

Conversation

jeanbisutti commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jeanbisutti commented Sep 6, 2022 •

edited

Loading