Runtime attachment feature #2325

jeanbisutti · 2022-06-13T16:38:45Z

Runtime attachment feature for Application Insights.

The feature delegates attachment to Otel runtime attachment project and allows to manage things specific to Application Insights (applicationinsights.json and version tracking).

A PR was also created to improve the Otel runtime attachment (more checks and logs).

...ing/src/main/java/com/microsoft/applicationinsights/agent/internal/init/FirstEntryPoint.java

heyams · 2022-06-13T17:11:54Z

...ntime-attach/src/main/java/com/microsoft/applicationinsights/attach/ApplicationInsights.java

+    RuntimeAttach.attachJavaagentToCurrentJVM();
+
+    if (!agentIsAttached()) {
+      System.setProperty(RUNTIME_ATTACHED_PROPERTY, "false");


can you explain why line 45 and then setting it to false here?

At this step, if the runtime attachment is not done, the agent.runtime.attached property always has a true value. So, for consistency it is set to false. However, since the agent is not attached, the property value won't be used.... So, I will remove this piece of code. The code will be easier to read without apparently any impact.

heyams · 2022-06-13T17:13:31Z

...ntime-attach/src/main/java/com/microsoft/applicationinsights/attach/ApplicationInsights.java

+    }
+  }
+
+  private static boolean agentIsAttached() {


I saw you have another pr on opentelemetry. should you wait till that PR is merged and use the upstream to check if agent is attached?

No, we have not to wait until the OTel PR is merged. OTel RuntimeAttach class exposes only one public method, attachJavaagentToCurrentJVM().

…uld be easier to understand)

agent/runtime-attach/README.md

...ing/src/main/java/com/microsoft/applicationinsights/agent/internal/init/FirstEntryPoint.java

agent/runtime-attach/README.md

agent/runtime-attach/build.gradle.kts

agent/runtime-attach/src/test/resources/applicationinsights.json

...ing/src/main/java/com/microsoft/applicationinsights/agent/internal/init/FirstEntryPoint.java

...ntime-attach/src/main/java/com/microsoft/applicationinsights/attach/ApplicationInsights.java

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

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

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

… not hierarchical when the application is packaged as a war or a jar

…g issue when the application is packaged as a war or jar file (other solution proposed in the Github issue)

...ntime-attach/src/main/java/com/microsoft/applicationinsights/attach/ApplicationInsights.java

heyams · 2022-06-14T20:02:14Z

...ntime-attach/src/main/java/com/microsoft/applicationinsights/attach/ApplicationInsights.java

+    boolean jsonFileNotFound = jsonUrl == null;
+    if (jsonFileNotFound) {


Suggested change

boolean jsonFileNotFound = jsonUrl == null;

if (jsonFileNotFound) {

if (jsonUrl == null) {

this is a style choice, I would have chosen to inline this too, but I also don't mind being flexible when it comes to style choices that aren't covered by spotless, errorprone, or https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/contributing/style-guideline.md

My intention with the jsonFileNotFound variable was to give meaning to the jsonUrl == null technical check.

heyams · 2022-06-14T20:04:38Z

agent/runtime-attach/src/test/resources/applicationinsights.json

@@ -0,0 +1,7 @@
+{


without this file, it will return Optional.empty. does the test still work without json config?

The RuntimeAttachTest test will fail if we remove this file because Application Insights needs a connection string provided in the json file. The connection string may also be supplied with a system property or an environment variable, without a json file, and the code execution path will then use the Optional.empty() line.

trask

👍

agent/runtime-attach/build.gradle.kts

...ntime-attach/src/main/java/com/microsoft/applicationinsights/attach/ApplicationInsights.java

agent/runtime-attach/src/test/resources/applicationinsights.json

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

…is runtime attached

lgtm-com · 2022-06-16T12:48:27Z

This pull request introduces 1 alert when merging aca7ecd into 7eb023e - view on LGTM.com

new alerts:

1 for Potential input resource leak

… real problem when the application is packaged as a war)

jeanbisutti · 2022-06-17T19:59:31Z

RuntimeAttach and AgentFileProvider classes could be removed once the 2 following OTel PRs will be merged and a new OTel release will be done:

trask

nice, a couple of suggestions on the internal property names if they make sense

...ntime-attach/src/main/java/com/microsoft/applicationinsights/attach/ApplicationInsights.java

…sights/attach/ApplicationInsights.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Runtime attachment feature

6dca374

jeanbisutti requested review from heyams and trask as code owners June 13, 2022 16:38

jeanbisutti marked this pull request as draft June 13, 2022 16:39

Fix code formatting

ec1b0e5

heyams reviewed Jun 13, 2022

View reviewed changes

jeanbisutti marked this pull request as ready for review June 13, 2022 17:21

jeanbisutti added 2 commits June 13, 2022 23:41

Remove final check on "agent.runtime.attached" property

a964338

Check at the beginning if the agent is already attached (the code sho…

4b5b7c2

…uld be easier to understand)

heyams reviewed Jun 13, 2022

View reviewed changes

agent/runtime-attach/README.md Outdated Show resolved Hide resolved

heyams reviewed Jun 13, 2022

View reviewed changes

agent/runtime-attach/README.md Show resolved Hide resolved

trask reviewed Jun 13, 2022

View reviewed changes

jeanbisutti and others added 13 commits June 14, 2022 11:43

Update code following a change on OTel PR

39b9755

Update prefix sdk name

c8c1fd5

Use placeholder in Readme

581fb23

Replace triple ` by single one in Readme

1fec2c0

Update agent/runtime-attach/src/test/resources/applicationinsights.json

a796083

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

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

3a1dfa8

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

Scope runtime attach property

748709e

Update agent/runtime-attach/build.gradle.kts

7062f58

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Remove hard coded dependency

a833cb5

Code formatting

15d3838

Gradle: add otelContribAlphaVersion

f3ee69e

Try another implementation to avoid .IllegalArgumentException: URI is…

a93b23e

… not hierarchical when the application is packaged as a war or a jar

Roll back real path solution because it does not solve the json confi…

041b339

…g issue when the application is packaged as a war or jar file (other solution proposed in the Github issue)

heyams reviewed Jun 14, 2022

View reviewed changes

trask approved these changes Jun 15, 2022

View reviewed changes

jeanbisutti and others added 3 commits June 15, 2022 15:53

Make ApplicationInsights final

709a4f7

Logger in lower case

1d5b9a6

Update agent/runtime-attach/src/test/resources/applicationinsights.json

4767eb2

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

jeanbisutti added 2 commits June 15, 2022 16:42

Dependency cleaning

5dbc01b

New code to be able to manage JSON configuration file when the agent …

aca7ecd

…is runtime attached

Close resources

e87460c

jeanbisutti marked this pull request as draft June 16, 2022 13:35

jeanbisutti added 10 commits June 16, 2022 16:17

Rollback last implementation (better understanding now of what is the…

803caf8

… real problem when the application is packaged as a war)

OTel property renamed

bb8fd7c

Change the mechanism to get the agent jar file

6613310

Fix code formatting

b78cc59

Fix code formatting

03345b1

Fix checkstyle

f9a0e4d

Fix "URI is not hierarchical" for JSON file

c40ad4b

Code formatting

1e51072

Use OTel attachment code with "URI is not hierarchical" (agent jar)

087f82c

Use code of 2 OTel PRs

e0d7414

jeanbisutti marked this pull request as ready for review June 17, 2022 20:00

jeanbisutti requested a review from trask June 17, 2022 20:00

trask approved these changes Jun 17, 2022

View reviewed changes

...ntime-attach/src/main/java/com/microsoft/applicationinsights/attach/ApplicationInsights.java Show resolved Hide resolved

...ntime-attach/src/main/java/com/microsoft/applicationinsights/attach/ApplicationInsights.java Outdated Show resolved Hide resolved

jeanbisutti and others added 7 commits June 17, 2022 22:57

Synchronize with last OTel code

01a5c9f

Update agent/runtime-attach/src/main/java/com/microsoft/applicationin…

7163df6

…sights/attach/ApplicationInsights.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Update agent/runtime-attach/src/main/java/com/microsoft/applicationin…

f1e0578

…sights/attach/ApplicationInsights.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Fix build

ca5b747

Use last OTel java contrib release

467fce7

Fix mistakes on configuration origins

c6531e4

Simplify code

1efa71d

jeanbisutti merged commit 37f6d4c into main Jun 20, 2022

jeanbisutti deleted the runtime-attach branch June 20, 2022 10:25

trask mentioned this pull request Jun 20, 2022

Ability to programmatically attach to own JVM #2125

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Runtime attachment feature #2325

Runtime attachment feature #2325

jeanbisutti commented Jun 13, 2022

heyams Jun 13, 2022

jeanbisutti Jun 13, 2022

heyams Jun 13, 2022

jeanbisutti Jun 13, 2022

heyams Jun 14, 2022

trask Jun 15, 2022

jeanbisutti Jun 15, 2022

heyams Jun 14, 2022

jeanbisutti Jun 15, 2022

trask left a comment

lgtm-com bot commented Jun 16, 2022

jeanbisutti commented Jun 17, 2022

trask left a comment

		boolean jsonFileNotFound = jsonUrl == null;
		if (jsonFileNotFound) {

	boolean jsonFileNotFound = jsonUrl == null;
	if (jsonFileNotFound) {
	if (jsonUrl == null) {

Runtime attachment feature #2325

Runtime attachment feature #2325

Conversation

jeanbisutti commented Jun 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trask left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 16, 2022

jeanbisutti commented Jun 17, 2022

trask left a comment

Choose a reason for hiding this comment