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

Move the jaeger thrift http exporter from core to contrib repo #137

Merged
merged 6 commits into from
Mar 31, 2020

Conversation

objectiser
Copy link
Contributor

Description:
This PR is to enact action item 3 from open-telemetry/opentelemetry-collector#618 (comment). Not sure if possible to preserve history, but this code was copied from here.

Link to tracking Issue:
Related to open-telemetry/opentelemetry-collector#618

Testing:
Uses existing tests copied from core repo.

Documentation:
Text from exporter README (jaeger thrift http section) has been extracted and added to a new README in the exporter's folder.

@objectiser
Copy link
Contributor Author

The testbed is currently failing due to the duplicate exporter for jaeger_thrift_http. Should I change the contrib exporter to jaeger_thrift?

@pjanotti
Copy link
Contributor

@objectiser just to be 100% sure: no actual code changes just taking the files as they are.

Regarding the error, we can rename it temporarily so we can do the merges on the repos independently, but before we release and tag another contrib we go back to jaeger_thrift_http that requires a new tag on core (to be done after the receiver is removed from there). The alternative is to remove first from core publish a new one and then merge this one. I prefer to do the former @tigrannajaryan @owais

@objectiser
Copy link
Contributor Author

just to be 100% sure: no actual code changes just taking the files as they are.

@pjanotti Yes that is correct.

Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

It actually LGTM @objectiser, just setting as "request changes" so we can coordinate correctly.

@pjanotti
Copy link
Contributor

@objectiser could you please push a change with jaeger_thrift so we can clean the build? This way we get ready to merge this one.

Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

LGTM, could you please rebase @objectiser

@objectiser
Copy link
Contributor Author

@pjanotti @owais ready to review/merge.

@objectiser
Copy link
Contributor Author

@pjanotti @owais please review/merge, thanks.

ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022
* Fix compilation with Golang 1.13

* run go mod tidy again
bogdandrutu pushed a commit that referenced this pull request May 12, 2022
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this pull request Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants