-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
NH-26615 Service Instance #70
Conversation
@@ -59,7 +72,6 @@ public static void initialize() throws InvalidConfigException { | |||
//future = executeStartupTasks(); //Cannot call this here, see https://github.com/appoptics/opentelemetry-custom-distro/issues/7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this line be removed completely? Or if good to keep for future, is there a way this method can be called with AutoConfiguredOpenTelemetrySdk otelSDK
to match sig update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that comment seems very much leftover from exploratory stage so I've removed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall lgtm! Just added one comment.
Thank you for posting the detailed example messages that match the updated spec! Great use of OTel Java's resource attributes and afterAgent to get those defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
initMessage.put(TELEMETRY_SDK_LANGUAGE.getKey(), "java"); | ||
} | ||
try { | ||
if (!initMessage.containsKey(PROCESS_RUNTIME_DESCRIPTION.getKey())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous code checked to make sure the system property returned in System.getProperty
wasn't null (eg java.version
). Should the uses of System.getProperty
continue to do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Certain system properties are always available so I've added defaults for ones that are not: java.runtime.name
, java.runtime.version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR looks good, just had one minor comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm! Thanks for the revisions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
Jira
https://swicloud.atlassian.net/browse/NH-26615
See also https://github.com/librato/joboe/pull/1572
Changes
sw.apm.service.key
system property if captured by Process Resource -- opted for simplest replacement of any char with****
instead of first/last four char, etc. since this information is not customer facing.Test
In a Linux container and the test collector configured to
console.STATUS.printHostId: true
, seeing the expected HostID (includinguuid
) and __Init message:__Init message pretty-printed:
Another run in a Linux container, when the env var
OTEL_JAVA_DISABLED_RESOURCE_PROVIDERS=io.opentelemetry.sdk.extension.resources.ProcessResourceProvider,io.opentelemetry.sdk.extension.resources.ProcessRuntimeResourceProvider
is used to disable Process and ProcessRuntime Resources in the agent, seeing newHostID.uuid
and the required __Init message attributes are still set:__Init message pretty-printed:
Another run in a Linux container, where the service key is passed in the command line as a Java system property (and otel debugging is enabled), e.g.
-Dsw.apm.service.key=top-secret:foo
and-Dotel.javaagent.debug=true
, seeing service key masked:Instrumented application log:
__Init message pretty-printed:
On a Windows EC2 instance with metadata disabled (to simulate on-prem host) and the test collector configured to
console.STATUS.printHostId: true
, seeing the expected HostID (includinguuid
) and __Init message:__Init message pretty-printed: