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 AWS resource detectors to java agent #1259

Closed
genebean opened this issue Sep 25, 2020 · 10 comments
Closed

Add AWS resource detectors to java agent #1259

genebean opened this issue Sep 25, 2020 · 10 comments
Labels
enhancement New feature or request

Comments

@genebean
Copy link

genebean commented Sep 25, 2020

It seems these exist but are not bundled into the agent.

@anuraaga
Copy link
Contributor

For reference, we're bundling these in the (future) AWS distribution of the agent

https://github.com/anuraaga/aws-opentelemetry-java-instrumentation

It could make sense to package them into the agent, though maybe packaging every single resource detection mechanism is too much? In theory, we could have all the resources implemented in the collector in the agent too but then it'd get really bloated so a bit worried about that.

@genebean
Copy link
Author

I can absolutely see the appeal of both sides. The repackaging is a good idea so long as those variations are referenced from the main project so that they are easy to discover by users

@trask
Copy link
Member

trask commented Sep 27, 2020

I don't think we're supposed to reference vendor distributions from this project.

We could generically document that vendor distributions exist, and mention they may support additional resource detectors, and point to https://opentelemetry.io/registry/, with some kind of generic search criteria built into the query string to narrow the results down to javaagent vendor distributions.

I do think we have the option to include additional resource detectors in the distribution from this repo, e.g. many vendors/users would benefit from AWS resource detectors.

I'm not sure the size of the additional resource detectors we are talking about. E.g. the xray and lightstep propagators we include are pretty small, so don't add much bloat.

@genebean
Copy link
Author

genebean commented Sep 27, 2020

I would assume that if vendor detectors were built into alternate packages that those packages would also live in this namespace.

I agree that, size permitting, including them here does make sense. FWIW, ruby includes them as do a few other languages. That said, ruby does package each as its own gem.

@iNikem
Copy link
Contributor

iNikem commented Sep 28, 2020

Seems like https://start.opentelemetry.io to me ;)

And then we will have to add ASCII banner to our agent as well.

@anuraaga
Copy link
Contributor

FWIW, ruby includes them as do a few other languages. That said, ruby does package each as its own gem.

Yeah, the Java SDK also publishes a maven artifact for them as an extension similar to this. I think bundling into the agent is a more tricky decision since it's forced into the published artifact rather than being chosen by the user.

For context, one of the reasons the AWS resource detectors for Java aren't huge is I removed their usage of the AWS SDK, otherwise they'd be megabytes :) They still have Jackson core, and may depend on Jackson databind soon unless I feel like removing that later. Not incredible, but as a general pattern I'd expect resource detectors to have more custom logic than propagators and a chance of being potentially large. Interestingly, the collector detectors all use the cloud SDKs, probably because Go doesn't have a problem of including unused code when only using a small part of a library.

It also feels a bit weird for our agent to include only AWS detectors, but not GCP, Azure etc. It'd be easier to decide if we had those available, we could see the general size, and could include a balanced, non-single-cloud-biased set of detectors. So I'd still lean a bit toward punting on including them until we can have more balanced coverage.

Agree that on the OTel side there might not be great discoverability of e.g., the AWS distribution. But AWS will be doing some heavy marketing so perhaps it will be ok :)

@iNikem
Copy link
Contributor

iNikem commented Jun 15, 2021

@anuraaga I think this is already done in SDK repo. Can we close this?

@anuraaga
Copy link
Contributor

I think we've discussed including the sdk-extension-aws into the agent by default since it's not tracing-vendor specific and pretty small. But ideally someone other than me pulls that trigger :P

@trask trask added the enhancement New feature or request label Jun 26, 2022
@SylvainJuge
Copy link
Contributor

As discussed yesterday in the Java SIG meeting, the AWS resource detection has been contributed to opentelemetry-java-contrib/aws-resources, so it seems relevant to close this issue for now as there is no plan to move it back to the instrumentation repository.

@zeitlinger
Copy link
Member

Is fixed by #10754

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants