Skip to content

Add jaeger thrift-http exporter#228

Merged
pjanotti merged 1 commit intoopen-telemetry:masterfrom
pjanotti:add-jaeger-thrift-http-exporter
Aug 5, 2019
Merged

Add jaeger thrift-http exporter#228
pjanotti merged 1 commit intoopen-telemetry:masterfrom
pjanotti:add-jaeger-thrift-http-exporter

Conversation

@pjanotti
Copy link
Copy Markdown
Contributor

@pjanotti pjanotti commented Aug 2, 2019

Adds the Jaeger Thrift over HTTP exporter.

Fixes #36
Opportunistically fixes #227 by removing remaining related code.

Adds the Jaeger Thrift over HTTP exporter.
@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #228 into master will increase coverage by 0.53%.
The diff coverage is 67.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
+ Coverage   72.79%   73.33%   +0.53%     
==========================================
  Files         110      110              
  Lines        6338     6365      +27     
==========================================
+ Hits         4614     4668      +54     
+ Misses       1491     1458      -33     
- Partials      233      239       +6
Impacted Files Coverage Δ
defaults/defaults.go 80% <100%> (+0.68%) ⬆️
...porter/jaeger/jaegerthrifthttpexporter/exporter.go 63.04% <63.04%> (ø)
...xporter/jaeger/jaegerthrifthttpexporter/factory.go 72.72% <72.72%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b39842b...ae71f8b. Read the comment docs.

Copy link
Copy Markdown
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

LGTM overall


// URL is the URL to send the Jaeger trace data to (e.g.:
// http://some.url:14268/api/traces).
URL string `mapstructure:"url"`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: can we use endpoint to be consistent with other configs?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We have been consistent on endpoint for receivers because they should always take the same syntax (host:port) but for exporters we had a different conversation in one previous PR: #207 (comment) let's know if you agree with that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

SGTM

@pjanotti pjanotti merged commit 1d38ba3 into open-telemetry:master Aug 5, 2019
@pjanotti pjanotti deleted the add-jaeger-thrift-http-exporter branch August 6, 2019 16:38
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
* Update metrics API to match current spec

* update options to match the spec

* drop the global meter API

* more docs

* get rid of leftover mentions about descriptor
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
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.

exporter: jaeger thrift-tchannel deprecation Add factory and new-style config for Jaeger exporter

4 participants