Auto-detect service name based on the jar name #6817

mateuszrzeszutek · 2022-10-06T15:01:35Z

No description provided.

mateuszrzeszutek · 2022-10-06T15:02:06Z

instrumentation/resources/library/build.gradle.kts

@@ -1,9 +1,8 @@
 plugins {
  id("otel.library-instrumentation")
-  id("otel.animalsniffer-conventions")


AFAIK SPI doesn't work on android anyway, so we can get rid of this plugin.

breedx-splk

Looks good. A couple of small nits but otherwise lovely. I think users will appreciate this!

breedx-splk · 2022-10-18T17:55:36Z

...ary/src/test/java/io/opentelemetry/instrumentation/resources/JarServiceNameProviderTest.java

+              "/path to app/with spaces/my-service.jar 1 2 3",
+              "/path to app/with spaces/my-service.jar"),
+          arguments(
+              "/path to app/with spaces/my-service 1 2 3", "/path to app/with spaces/my-service"));


This one is a little surprising to me because it doesn't actually have .jar in any argument names. Since we're not looking for .jar, how are we able to confidently assume that the file is my-service and not my-service 1 2 3?

That's what the second arg is for - the service detector checks whether a particular path (a substring of the sun.java.command) is a regular file (not a directory; a leaf in the file system graph).

breedx-splk · 2022-10-18T18:00:50Z

...rces/library/src/main/java9/io/opentelemetry/instrumentation/resources/ProcessArguments.java

+  static String[] getProcessArguments() {
+    return ProcessHandle.current().info().arguments().orElseGet(() -> new String[0]);
+  }


Leveraging the module system here is sooooooo much nicer than the reflection in the original version. ❤️

breedx-splk · 2022-10-18T18:05:12Z

...ary/src/test/java/io/opentelemetry/instrumentation/resources/JarServiceNameProviderTest.java

+    String[] args =
+        new String[] {"-Dtest=42", "-Xmx666m", "-jar", "/path/to/app/my-service.jar", "abc", "def"};
+    JarServiceNameProvider serviceNameProvider =
+        new JarServiceNameProvider(() -> args, prop -> null, file -> false);


This gave me a little bit of pause because the "does the file exist" check is returning false here....and the test passes because it's not used. Definitely prefer passing null for tests where the argument is not used (there are a few others too I believe).

How about () -> throw new AssertionError() instead? Passing null results in null warnings

We should allow passing null in tests!! I think that suggestion is an improvement, though.

breedx-splk · 2022-10-18T18:05:55Z

...ain/java/io/opentelemetry/instrumentation/spring/resources/SpringBootServiceNameGuesser.java

@@ -90,7 +92,7 @@ public Resource createResource(ConfigProperties config) {
        .findFirst()
        .map(
            serviceName -> {
-              logger.log(Level.FINER, "Guessed Spring Boot service name: {0}", serviceName);
+              logger.log(FINE, "Auto-detected Spring Boot service name: {0}", serviceName);


Fancy phrasing is fancy. 🎩

I wanted the message to be very similar in all of these service.name auto-detectors -- thus this change.

👍 I like auto-detected > guessed 😄

EDIT: rename class SpringBootServiceNameDetector?

I renamed both classes to ...Detector

👍 I like auto-detected > guessed 😄

I mean, it's more official-sounding for sure. But let's be real, it's only a heuristic, and it's guessing. 😁

oh "Heuristic" is another good fancy word 😂. Guess sounds like it's sorta random.

instrumentation/resources/library/build.gradle.kts

trask · 2022-10-19T21:47:50Z

...ary/src/test/java/io/opentelemetry/instrumentation/resources/JarServiceNameProviderTest.java

+          arguments("/path/to/my-service.jar", "/path/to/my-service.jar"),
+          arguments(
+              "/path to app/with spaces/my-service.jar 1 2 3",
+              "/path to app/with spaces/my-service.jar"),
+          arguments(
+              "/path to app/with spaces/my-service 1 2 3", "/path to app/with spaces/my-service"));


I was trying this out, and these tests fail on Windows, because later on the Path gets normalized with \, it's ok to merge as-is though (I need to try again to get the github actions running on windows)

Oh, I completely didn't think of this. Great point, I've changed this test to use Path instead, hope that helps 🤞

trask · 2022-10-19T21:48:47Z

...ain/java/io/opentelemetry/instrumentation/spring/resources/SpringBootServiceNameGuesser.java

@@ -90,7 +92,7 @@ public Resource createResource(ConfigProperties config) {
        .findFirst()
        .map(
            serviceName -> {
-              logger.log(Level.FINER, "Guessed Spring Boot service name: {0}", serviceName);
+              logger.log(FINE, "Auto-detected Spring Boot service name: {0}", serviceName);


👍 I like auto-detected > guessed 😄

EDIT: rename class SpringBootServiceNameDetector?

...library/src/main/java/io/opentelemetry/instrumentation/resources/JarServiceNameProvider.java

trask · 2022-10-19T21:55:11Z

...library/src/main/java/io/opentelemetry/instrumentation/resources/JarServiceNameProvider.java

+    String serviceName = config.getString("otel.service.name");
+    Map<String, String> resourceAttributes = config.getMap("otel.resource.attributes");


are these needed b/c this resource provider can't run after the built-in resource provider?

Yes -- the EnvironmentResource always runs last, as it's not a ResourceProvider, it's just called after all of the providers.

do you think this is changeable, to make otel.service.name and otel.resource.attributes be real resource detectors, so that they can be leveraged as part of shouldApply (cc @jack-berg)

(I think this is a perfectly good workaround tho, and hopefully it's not a common use case)

do you think this is changeable, to make otel.service.name and otel.resource.attributes be real resource detectors, so that they can be leveraged as part of shouldApply

This idea has definitely cross my mine. I've also considered taking the idea further and modeling all the exporter configuration as Configurable{Signal}ExporterProviders. I think it could result in more module / testable code, and potentially have some nice interactions with a future file based configuration scheme.

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

mateuszrzeszutek requested a review from a team as a code owner October 6, 2022 15:01

mateuszrzeszutek commented Oct 6, 2022

View reviewed changes

mateuszrzeszutek force-pushed the jar-service-name branch from 96e9de1 to 780b234 Compare October 6, 2022 15:04

breedx-splk approved these changes Oct 18, 2022

View reviewed changes

Mateusz Rzeszutek added 2 commits October 19, 2022 12:01

Auto-detect service name based on the jar name

5a5bee8

code review comments

5eccd69

mateuszrzeszutek force-pushed the jar-service-name branch from 780b234 to 5eccd69 Compare October 19, 2022 10:16

trask reviewed Oct 19, 2022

View reviewed changes

Mateusz Rzeszutek and others added 2 commits October 20, 2022 16:05

Apply suggestions from code review

d34c0a1

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

code review comments

f4576f3

trask approved these changes Oct 21, 2022

View reviewed changes

trask merged commit 33b0688 into open-telemetry:main Oct 23, 2022

mateuszrzeszutek deleted the jar-service-name branch October 23, 2022 21:43

LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 31, 2022

Auto-detect service name based on the jar name (open-telemetry#6817)

9b16622

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

LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Dec 4, 2022

Auto-detect service name based on the jar name (open-telemetry#6817)

a1ed629

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto-detect service name based on the jar name #6817

Auto-detect service name based on the jar name #6817

mateuszrzeszutek commented Oct 6, 2022

mateuszrzeszutek Oct 6, 2022

breedx-splk left a comment

breedx-splk Oct 18, 2022

mateuszrzeszutek Oct 19, 2022

breedx-splk Oct 18, 2022

breedx-splk Oct 18, 2022

mateuszrzeszutek Oct 19, 2022

breedx-splk Oct 20, 2022

breedx-splk Oct 18, 2022

mateuszrzeszutek Oct 19, 2022

mateuszrzeszutek Oct 19, 2022

trask Oct 19, 2022

mateuszrzeszutek Oct 20, 2022

breedx-splk Oct 20, 2022

trask Oct 20, 2022

trask Oct 19, 2022

mateuszrzeszutek Oct 20, 2022

trask Oct 19, 2022

trask Oct 19, 2022

mateuszrzeszutek Oct 20, 2022

trask Oct 20, 2022

jack-berg Oct 20, 2022

		String serviceName = config.getString("otel.service.name");
		Map<String, String> resourceAttributes = config.getMap("otel.resource.attributes");

Auto-detect service name based on the jar name #6817

Auto-detect service name based on the jar name #6817

Conversation

mateuszrzeszutek commented Oct 6, 2022

Choose a reason for hiding this comment

breedx-splk left a 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

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

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