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 in starter: annotations #10613

Merged
merged 2 commits into from
Feb 21, 2024

Conversation

zeitlinger
Copy link
Member

@zeitlinger zeitlinger commented Feb 20, 2024

add in starter: annotations (but remove the old annotation lib)

@zeitlinger zeitlinger self-assigned this Feb 20, 2024
@zeitlinger zeitlinger requested a review from a team February 20, 2024 16:37
implementation(project(":instrumentation:resources:library"))
implementation("io.opentelemetry:opentelemetry-extension-trace-propagators")
implementation("io.opentelemetry.contrib:opentelemetry-aws-xray-propagator")
Copy link
Member

Choose a reason for hiding this comment

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

It is an extension located in the Java contrib repo (https://github.com/open-telemetry/opentelemetry-java-contrib). So, it would not make sense to enable it from a project located in the Java instrumentation Github repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

javaagent is also including it - and the docs mention it as one of the values.

Copy link
Member

Choose a reason for hiding this comment

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

Could you please elaborate with links

Copy link
Member Author

Choose a reason for hiding this comment

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

implementation("io.opentelemetry.contrib:opentelemetry-aws-xray-propagator")

Copy link
Contributor

Choose a reason for hiding this comment

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

Agent includes it because we have instrumentations that use this propagator. On the one hand including it is fine because the propagator is small and doesn't have dependencies. On the other hand getting rid of it later on might be difficult once the starter is stable. I'd personally not include it and I would also not include opentelemetry-extension-trace-propagators. The code in https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/propagators/CompositeTextMapPropagatorFactory.java already treats these as optional dependencies, I see no compelling reason for changing this.
You may wish to raise this on the SIG meeting if you believe that these should be included.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's not a priority for me - we'll just document that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@laurit I removed the resource propagators

@zeitlinger zeitlinger changed the title add in starter: resource propagators, annotations add in starter: annotations Feb 21, 2024
@laurit laurit merged commit ee9cdc3 into open-telemetry:main Feb 21, 2024
46 of 47 checks passed
@zeitlinger zeitlinger deleted the spring-starter-dependencies branch February 21, 2024 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants