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

Add declarative config support for resource providers #12144

Merged
merged 4 commits into from
Sep 12, 2024

Conversation

jack-berg
Copy link
Member

Implement the declarative configuration ComponentProvider<Resource> SPI now that support has been added for resources in open-telemetry/opentelemetry-java#6625.

@jack-berg jack-berg requested a review from a team August 29, 2024 20:57
@github-actions github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Aug 29, 2024
@@ -35,140 +19,9 @@
*/
public final class HostIdResourceProvider implements ConditionalResourceProvider {
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 class didn't follow the pattern of the others. Instead of having a Resource HostIdResource.get() method for calling directly by users, and a separate HostIdResourceProvider for integration with autoconfigure, it only had autoconfigure support.

I've refactored it to follow the pattern used elsewhere, which makes it easy to add a ComponentProvider implementation in HostIdResourceComponentProvider.

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed that there is no documentation for the JarServiceNameDetector or ManifestResourceProvider, and that these can't be used outside of autoconfigure.

@zeitlinger, since you wrote ManifestResourceProvider, were these omissions intentional?

Copy link
Member

Choose a reason for hiding this comment

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

no, I didn't think about this use case

* href="https://github.com/open-telemetry/semantic-conventions/blob/main/docs/resource/host.md#non-privileged-machine-id-lookup">the
* semantic conventions</a>
*/
public final class HostIdResource {
Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing new in this class - just breaking out HostIdResourceProvider into two classes.

@@ -8,7 +8,8 @@ val dependencyVersions = hashMapOf<String, String>()
rootProject.extra["versions"] = dependencyVersions

// this line is managed by .github/scripts/update-sdk-version.sh
val otelSdkVersion = "1.41.0"
// TODO: revert before merging
val otelSdkVersion = "1.42.0-SNAPSHOT"
Copy link
Member

Choose a reason for hiding this comment

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

reminder

Copy link
Member Author

Choose a reason for hiding this comment

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

Will resolve after #12186 is merged

@trask trask merged commit 2a7a553 into open-telemetry:main Sep 12, 2024
56 checks passed
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.

4 participants