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

Hosting vendor supported exporters for OpenTelemetry #1335

Closed
EdZou opened this issue Jul 22, 2020 · 7 comments
Closed

Hosting vendor supported exporters for OpenTelemetry #1335

EdZou opened this issue Jul 22, 2020 · 7 comments
Labels
question User is asking a question not related to a new feature or bug

Comments

@EdZou
Copy link
Contributor

EdZou commented Jul 22, 2020

AWS XRay Components

Description:

Hello everyone, I am an AWS intern working on adding Id generator, propagators and resource components for AWS X-Ray Exporter under JavaScript SDK repository.

Similar components exist in the Java implementation:

Id Generator: https://github.com/open-telemetry/opentelemetry-java/blob/a59748a904e0cb1e2e4f8df50dd346a82f26ff1e/sdk_extensions/aws_v1_support/src/main/java/io/opentelemetry/sdk/extensions/trace/aws/AwsXRayIdsGenerator.java#L36
Propagators: https://github.com/open-telemetry/opentelemetry-java/blob/a59748a904e0cb1e2e4f8df50dd346a82f26ff1e/extensions/trace_propagators/src/main/java/io/opentelemetry/extensions/trace/propagation/AwsXRayPropagator.java#L51
Resources: https://github.com/open-telemetry/opentelemetry-java/blob/a59748a904e0cb1e2e4f8df50dd346a82f26ff1e/sdk_extensions/aws_v1_support/src/main/java/io/opentelemetry/sdk/extensions/trace/aws/resource/AwsResource.java#L26

Questions:

  1. An existing implementation of id generator is here: https://github.com/open-telemetry/opentelemetry-js/blob/e9b2cf9aeb1daf5ffbab800681bfe1cafc636576/packages/opentelemetry-core/src/platform/node/id.ts. My questions are:
    1. The existing implementation only providing functions but not an IdGenerator interface in Go and Java implementation. So I also added an IdGenerator interface for JS repo and drafted a PR: Feat: Make ID generator configurable #1331. My question is under which folder should I place the IdGenerator interface? “opentelemetry-api/src/platform/node” or like “opentelemetry-core/src/trace”
    2. Also, if I implement AWS IdGenerator (either an interface or exported functions as existing implementation), where should I put this part of code?
  2. If I want add an AWS X-Ray Propagator, where should I add it? Should I add this to the opentelemetry-js-contrib/propagator (https://github.com/open-telemetry/opentelemetry-js-contrib/tree/master/propagators) folder? Like the discussion here: Is it acceptable for AWS x-ray propagators to be hosted by the OpenTelemetry project? opentelemetry-specification#637 (comment)
  3. Similarly for resources, which repository are I supposed to add AWS Xray resources component? I did not see vendor repo for resource in both opentelemetry-js and opentelemetry-js-contrib repo.
@vmarchaud
Copy link
Member

vmarchaud commented Jul 22, 2020

Similarly for resources, which repository are I supposed to add AWS Xray resources component? I did not see vender repo for resource in both opentelemetry-js and opentelemetry-js-contrib repo.

I believe it should be hosted in seperate repo, see #890

The existing implementation only providing functions but not an IdGenerator interface in Go and Java implementation. So I also added an IdGenerator interface for JS repo and drafted a PR: #1331. My question is under which folder should I place the IdGenerator interface? “opentelemetry-api/src/platform/node” or like “opentelemetry-core/src/trace”

Not sure but i believe the interface (and the default implementation) should be in opentelemetry-core. The custom implementation for AWS should be done in a seperate repo and given as an option for the tracer.

If I want add an AWS X-Ray Propagator, where should I add it

I think we would need to wait for open-telemetry/opentelemetry-specification#637 (comment) to have an answer

@vmarchaud vmarchaud added the question User is asking a question not related to a new feature or bug label Jul 22, 2020
@dyladan
Copy link
Member

dyladan commented Jul 22, 2020

Similarly for resources, which repository are I supposed to add AWS Xray resources component? I did not see vender repo for resource in both opentelemetry-js and opentelemetry-js-contrib repo.

I believe it should be hosted in seperate repo, see #890

It depends what is meant by "Resources". Is this simply resource detection, or some reporting of resources to x-ray? If it is detection, then the detector can live in the contrib repo. I have talked with other maintainers about this a few times and it seems like resource detection and possibly propagators may live in contrib, but that exporters should be hosted by the third party. Currently, the detectors live in our core resources package, but according to this, we should split them into their own packages.

The existing implementation only providing functions but not an IdGenerator interface in Go and Java implementation. So I also added an IdGenerator interface for JS repo and drafted a PR: #1331. My question is under which folder should I place the IdGenerator interface? “opentelemetry-api/src/platform/node” or like “opentelemetry-core/src/trace”

Not sure but i believe the interface (and the default implementation) should be in opentelemetry-core. The custom implementation for AWS should be done in a seperate repo and given as an option for the tracer.

I would add the interface to opentelemetry-core/src/trace/types.ts

If I want add an AWS X-Ray Propagator, where should I add it

I think we would need to wait for open-telemetry/opentelemetry-specification#637 (comment) to have an answer

I think it is probably ok for propagators to live in the contrib repo.

@EdZou
Copy link
Contributor Author

EdZou commented Jul 24, 2020

Hello Daniel! @dyladan Really thank you for your time!
As we discussed on the SIG meeting this week, we may also open a folder for AWS Xray IdGenerator, my question is will JS group open a "js-contrib/idGenerator" folder for id generator components? So we could add a "js-contrib/idGenerator/AWSXRayIdGenerator"? Or elsewhere should we add a folder for it?
Also, just to make sure we are going to add "js-contrib/propagator/AWSXrayPropagator" folder for AWS Xray propagator.

@dyladan
Copy link
Member

dyladan commented Jul 24, 2020

Empty folders cannot be committed to github so the first id generator package will need to create it.
I think the names propagators and id-generators work fine.

@EdZou
Copy link
Contributor Author

EdZou commented Jul 24, 2020

Got it, thank you!

@EdZou
Copy link
Contributor Author

EdZou commented Jul 28, 2020

Final question for this issue.
I now know the exact requirement for AWS resources components, I am going to add AWS Beanstalk and Ecs resource detector, am I supposed to add code under opentelemetry-resources/src/platform/node? @dyladan

@dyladan
Copy link
Member

dyladan commented Jul 29, 2020

@EdZou unless it is time-sensitive, I would wait for the changes required by open-telemetry/oteps#111 to land.

Specifically, resource detectors will be made to live in separate extension modules rather than living in our @opentelemetry/resources package.

@EdZou EdZou closed this as completed Aug 5, 2020
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this issue Dec 15, 2023
Co-authored-by: Amir Blum <amirgiraffe@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question User is asking a question not related to a new feature or bug
Projects
None yet
Development

No branches or pull requests

3 participants