Runtime attachment feature#2325
Conversation
| RuntimeAttach.attachJavaagentToCurrentJVM(); | ||
|
|
||
| if (!agentIsAttached()) { | ||
| System.setProperty(RUNTIME_ATTACHED_PROPERTY, "false"); |
There was a problem hiding this comment.
can you explain why line 45 and then setting it to false here?
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| private static boolean agentIsAttached() { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
No, we have not to wait until the OTel PR is merged. OTel RuntimeAttach class exposes only one public method, attachJavaagentToCurrentJVM().
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)
| boolean jsonFileNotFound = jsonUrl == null; | ||
| if (jsonFileNotFound) { |
There was a problem hiding this comment.
| boolean jsonFileNotFound = jsonUrl == null; | |
| if (jsonFileNotFound) { | |
| if (jsonUrl == null) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
My intention with the jsonFileNotFound variable was to give meaning to the jsonUrl == null technical check.
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
without this file, it will return Optional.empty. does the test still work without json config?
There was a problem hiding this comment.
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.
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
|
This pull request introduces 1 alert when merging aca7ecd into 7eb023e - view on LGTM.com new alerts:
|
… real problem when the application is packaged as a war)
trask
left a comment
There was a problem hiding this comment.
nice, a couple of suggestions on the internal property names if they make sense
…sights/attach/ApplicationInsights.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
…sights/attach/ApplicationInsights.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
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).