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

Jaeger exporter should be its own go module #205

Closed
iredelmeier opened this issue Oct 14, 2019 · 7 comments · Fixed by #237
Closed

Jaeger exporter should be its own go module #205

iredelmeier opened this issue Oct 14, 2019 · 7 comments · Fixed by #237
Labels
good first issue Good for newcomers
Milestone

Comments

@iredelmeier
Copy link
Member

Among other things, this would decouple the core code from all the Jaeger dependencies, such as Thrift.

@eran-levy
Copy link
Contributor

@rghetia @iredelmeier will be happy to contribute the new module, can I just reference the new module repo here?

@rghetia
Copy link
Contributor

rghetia commented Oct 21, 2019

@eran-levy , yes. Thanks for contributing.

@eran-levy
Copy link
Contributor

eran-levy commented Oct 21, 2019

@rghetia ok will ref soon, wanted to make sure I document the way to generate the thrift clients that we have under internal/gen-go/jaeger - which jaeger version are we compatible with? when I generated using the Makefile that available at: https://github.com/jaegertracing/jaeger/tree/v1.14.0 I didnt see the same files that we have under our internal/gen-go/jaeger folder, how did you generate them? or you just combined them into the one file?

@rghetia
Copy link
Contributor

rghetia commented Oct 21, 2019

@eran-levy I did not generate those files. I had simply ported jaeger exporter from opencensus jaeger exporter v0.1.0.
If jaeger client library is used then gen-go files are not required. See PR#7. Using jaeger client library was discussed in #128.

eran-levy pushed a commit to eran-levy/opentelemetry-go-exporter-jaeger that referenced this issue Oct 22, 2019
@eran-levy
Copy link
Contributor

eran-levy commented Oct 22, 2019

@rghetia ok np, so here is the repo: https://github.com/eran-levy/opentelemtry-go-exporter-jaeger
let me know after you fork this repo into https://github.com/open-telemetry and then I will commit the changes to: https://github.com/open-telemetry/opentelemetry-go

the discussions around the jaeger go client libs are interesting and i agree that we shall have a single strategy re our clients usage. Currently, i will leave it as is with gen-go as you ported.
Obviously will be very happy to contribute to that as well.

@rghetia
Copy link
Contributor

rghetia commented Oct 22, 2019

@eran-levy there may be some confusion here. While we have not decided where the exporter would reside, but we want the exporters to be its own module. Similar to examples (example/http, example/basic). Later when we have a place (another repo or different directory within the same repo) we will have another PR to do the change.

@eran-levy
Copy link
Contributor

@rghetia ok, tnx, see PR #237

rghetia pushed a commit that referenced this issue Oct 24, 2019
* Jaeger exporter should be its own go module #205

* fix review comments and build #205

* resolve mod conflicts #205
hstan referenced this issue in hstan/opentelemetry-go Oct 15, 2020
* Add Config struct definition, test files, and testing data

* Add tests for Validate method

* Implement Validate method

* Add Config utility files and testing data

* Add helper function to create YAML files for testing

* Add NewConfig function, Options interface, and tests

* Add Viper tags to Config struct and two With functions

* Add test for WithFilepath

* Add WithClient and test for WithClient

* Remove default for http client and adjust tests

* Add check for conflicting authentication types and update tests

* Update README.md

* Run make precommit

* Add example Config struct

* Add NewRawExporter for creating an Exporter and TestNewRawExporter

* Add NewExportPipeline for creating a controller and TestNewExportPipeline

* Add InstallNewPipeline and TestInstallNewPipeline

* Update README and run make precommit
shbieng pushed a commit to shbieng/opentelemetry-go that referenced this issue Aug 26, 2022
* Jaeger exporter should be its own go module open-telemetry/opentelemetry-go#205

* fix review comments and build #205

* resolve mod conflicts #205
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants