Skip to content
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

detect service.name based on build-info.properties #10515

Conversation

zeitlinger
Copy link
Member

@zeitlinger zeitlinger commented Feb 11, 2024

Add new resource provider SpringBootBuildInfoServiceNameDetector.

@zeitlinger zeitlinger requested a review from a team as a code owner February 11, 2024 10:47
@zeitlinger zeitlinger self-assigned this Feb 11, 2024
@zeitlinger zeitlinger force-pushed the spring-service-name-from-build-info branch from 2400cdb to 545823f Compare February 12, 2024 09:29
@jeanbisutti
Copy link
Member

The current documented bhavior is

image

Could you please document the new proposed behavior somewhere for discussion in the next Java SIG meeting?

@zeitlinger zeitlinger force-pushed the spring-service-name-from-build-info branch from 545823f to bff1146 Compare February 12, 2024 11:21
@zeitlinger
Copy link
Member Author

Could you please document the new proposed behavior somewhere for discussion in the next Java SIG meeting?

added to this PR

@zeitlinger zeitlinger force-pushed the spring-service-name-from-build-info branch 2 times, most recently from 3b4b952 to 40056cf Compare February 12, 2024 12:20
@jeanbisutti
Copy link
Member

Could you please document the new proposed behavior somewhere for discussion in the next Java SIG meeting?

added to this PR

The build-info.properties file seems always generated when the Spring Boot Maven/Gradle plugin creates an executable JAR:

build.artifact=spring-petclinic
build.encoding.reporting=UTF-8
build.encoding.source=UTF-8
build.group=org.springframework.samples
build.java.source=17
build.java.target=17
build.name=petclinic
build.time=2023-05-10T07\:42\:50Z
build.version=3.1.0-SNAPSHOT

It could be used instead of the detector based on the JAR file name, and the version may ne deduced from build.version.

Another option may be to use the manifest file.

@zeitlinger
Copy link
Member Author

The build-info.properties file seems always generated when the Spring Boot Maven/Gradle plugin creates an executable

no, you have to trigger that explicitly

springBoot {
  buildInfo {
  }
}

It could be used instead of the detector based on the JAR file name, and the version may ne deduced from build.version.

yes, that's why I'm proposing to downgrade the priority of the jar detector: #10523

Another option may be to use the manifest file.

If you want to do something manually, I'd suggest to use otel.resource.attributes

@jeanbisutti
Copy link
Member

no, you have to trigger that explicitly

springBoot {
  buildInfo {
  }
}

I am surprised. I have tested with the Spring Boot Maven plugin and the build info file is generated without any additional configuration.

@jeanbisutti
Copy link
Member

no, you have to trigger that explicitly

springBoot {
  buildInfo {
  }
}

I am surprised. I have tested with the Spring Boot Maven plugin and the build info file is generated without any additional configuration.

Agree for Maven

@zeitlinger
Copy link
Member Author

I am surprised. I have tested with the Spring Boot Maven plugin and the build info file is generated without any additional configuration.

My bad - the smoke test runs without this as well.

Copy link
Member

@jeanbisutti jeanbisutti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please fix the merge conflict?

@zeitlinger zeitlinger force-pushed the spring-service-name-from-build-info branch from 06c22b6 to 416a038 Compare February 14, 2024 14:49
@zeitlinger zeitlinger marked this pull request as draft February 14, 2024 16:38
@zeitlinger zeitlinger marked this pull request as ready for review February 14, 2024 18:25
@zeitlinger
Copy link
Member Author

Could you please document the new proposed behavior somewhere for discussion in the next Java SIG meeting?

open-telemetry/opentelemetry.io#3999

@zeitlinger
Copy link
Member Author

@jeanbisutti can you review? (I know we talked about it, but I don't remember if anything needs to be done)

@zeitlinger zeitlinger force-pushed the spring-service-name-from-build-info branch from 6e34bfa to ffb7cd3 Compare February 16, 2024 17:59
@zeitlinger
Copy link
Member Author

I added an integration test now (smoke test), which is not a spring starter test.
I couldn't figure out why it fails - the build-info file is generated correctly.

@zeitlinger
Copy link
Member Author

I added an integration test now (smoke test), which is not a spring starter test. I couldn't figure out why it fails - the build-info file is generated correctly.

Now I got it - the smoke test image is referred to by a tag - so it's not using the image generated in the current build.

@jeanbisutti
Copy link
Member

e smoke test image

The smoke tests based on a Docker image are used to test the Java agent, not the OpenTelemetry Spring starter.

@@ -1,2 +1 @@
build.artifact=something
build.name=some-name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cold you please explain the removal of this line?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a test file that is supposed to not find the expected entry.
Before, it did not have build.version - and now it also must not have build.name.

Copy link
Member

@jeanbisutti jeanbisutti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would need to verify it works with GraalVM.

import java.util.Properties;
import java.util.logging.Logger;

public abstract class SpringBootBuildInfoDetector implements ConditionalResourceProvider {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather avoid inheritance and do composition. But it's not a blocker for this case that keeps simple.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's a good point - wdyt about using the mechanism from #10621 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my point of view it could be great, but only for new classes.

@zeitlinger zeitlinger force-pushed the spring-service-name-from-build-info branch from 145660f to 751c5bd Compare March 5, 2024 08:38
@github-actions github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Mar 5, 2024
@zeitlinger zeitlinger modified the milestone: v2.2.0 Mar 11, 2024
@zeitlinger zeitlinger marked this pull request as draft April 11, 2024 06:51
@zeitlinger
Copy link
Member Author

is outdated now - re-open if needed

@zeitlinger zeitlinger closed this Apr 16, 2024
@zeitlinger zeitlinger deleted the spring-service-name-from-build-info branch April 16, 2024 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test native This label can be applied to PRs to trigger them to run native tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants