Add jaeger grpc exporter #219

pjanotti · 2019-08-01T17:42:39Z

Adds a Jaeger gRPC exporter.

Note: the design document for the configuration doesn't mention a protocol section for exporters, however, in the the section for default config in lists a protocols under exporters but I'm considering that to be a spurious copy from receivers.

pjanotti · 2019-08-01T17:43:14Z

exporter/jaeger/jaeger_thrift_http_sender.go

@@ -35,29 +35,29 @@ import (
 // Default timeout for http request in seconds
 const defaultHTTPTimeout = time.Second * 5

-// JaegerThriftHTTPSender forwards spans encoded in the jaeger thrift
+// ThriftHTTPSender forwards spans encoded in the jaeger thrift


Renamed to pass lint.

pjanotti · 2019-08-01T17:45:44Z

exporter/jaeger/jaeger_thrift_tchannel_sender.go

 // out on tchannel in thrift encoding
-type JaegerThriftTChannelSender struct {
+type ThriftTChannelSender struct {


Renamed to pass lint.

exporter/jaeger/jaegergrpcexporter/config.go

codecov-io · 2019-08-01T17:55:43Z

Codecov Report

❗ No coverage uploaded for pull request base (master@fdb5047). Click here to learn what that means.
The diff coverage is 68.08%.

@@            Coverage Diff            @@
##             master     #219   +/-   ##
=========================================
  Coverage          ?   72.79%           
=========================================
  Files             ?      110           
  Lines             ?     6338           
  Branches          ?        0           
=========================================
  Hits              ?     4614           
  Misses            ?     1491           
  Partials          ?      233

Impacted Files	Coverage Δ
exporter/jaeger/jaeger_thrift_tchannel_sender.go	`0% <0%> (ø)`
exporter/jaeger/jaeger_thrift_http_sender.go	`0% <0%> (ø)`
defaults/defaults.go	`79.31% <100%> (ø)`
exporter/jaeger/jaegergrpcexporter/factory.go	`80.95% <80.95%> (ø)`
exporter/jaeger/jaegergrpcexporter/exporter.go	`82.6% <82.35%> (ø)`

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 fdb5047...f3a81ef. Read the comment docs.

songy23

A few minor questions, otherwise LGTM.

songy23 · 2019-08-01T22:53:27Z

exporter/jaeger/jaegergrpcexporter/exporter.go

+// New returns a new Jaeger gRPC exporter.
+// The exporter name is the name to be used in the observability of the exporter.
+// The collectorEndpoint should be of the form "hostname:14250" (a gRPC target).
+func New(exporterName, collectorEndpoint string) (exporter.TraceExporter, error) {


Nit: do we want to use zap.Logger in this exporter?

At this stage the observability is covered via metrics but the main point is to do any added observability on the exporterhelper. Notice that on the typical usage that includes a queued-retry processor there will be logging for errors and retries.

songy23 · 2019-08-01T22:54:13Z

exporter/jaeger/jaegergrpcexporter/config.go

+)
+
+// Config defines configuration for Jaeger gRPC exporter.
+type Config struct {


Should we leave a TODO for TLS option in the config?

Yes, I will create an issues to track not TLS but also other missing configurations: timeout, headers, and so on.

Actually there is already one issue to TLS to jaeger receiver and exporter: #126

tigrannajaryan · 2019-08-02T17:16:45Z

exporter/jaeger/jaegergrpcexporter/config_test.go

+
+	"go.uber.org/zap"
+
+	"github.com/open-telemetry/opentelemetry-service/config"


Import ordering.

Adds a Jaeger gRPC exporter.

* Migrate updates to Prometheus receiver from opencensus-service * Remove unnecessary configuration for adjusting metrics * Update exporters README (#210) Remove stale content, put in place a simple skeleton, and put information for the Zipkin exporter. * Added Jaeger gRPC receiver (#197) * Added Jaeger gRPC receiver Signed-off-by: Juraci Paixão Kröhling <[email protected]> * Addressed first review round Signed-off-by: Juraci Paixão Kröhling <[email protected]> * Changes based on the feedback from the second review Signed-off-by: Juraci Paixão Kröhling <[email protected]> * Fixed import order, check for same ip:port on endpoints Signed-off-by: Juraci Paixão Kröhling <[email protected]> * Reverted the port-check Signed-off-by: Juraci Paixão Kröhling <[email protected]> * Fix README - remove broken links and commands (#211) Signed-off-by: Hui Kang <[email protected]> * Refactor opencensus exporter to make it easily extensible (#212) Refactored oc exporter code to make it easier to import in derived builds without copying all the code. Example derived exporter: https://github.com/owais/example-derived-oc-exporter - Moved internal/compression to root - Split opencensusexporter factory's `CreateTraceExporter` method into - `OCAgentOptions` and `CreateOCAgent` * Use sha256 instead of md5 in node batcher processor (#202) MD5 is insecure cryptographic hash functions, and are therefore unsuitable for security applications. * Add goimports check and fix import order for all files (#209) * Add goimports check and fix import order for all files Updates #155. * Try specifying a version for goimports * Show the diff instead of files and fix import * Fix default in Makefile * Add factory and update config for tail sampling processor (#200) * Add factory and update config for tail sampling processor Updates #146 * Move to package processor * Remove an unnecessary check * Move CreateDefaultConfig to factory and add unit tests * Fix test failure * Remove commented code * Add misspell check and fix all typos (#214) * Add misspell check and fix all typos Updates #155. * Format * Include yaml files * Move tail sampling config to its own package and remove stale configs (#217) Follow up of #200. Second step of #146. * Add Observability Vision document (#188) * Add Observability Vision document This is a guidance document that developers can consult with when working on improving own observability of OpenTelemetry Service. * PR fixes * Add Zipkin exporter factory (#207) * Add Zipkin exporter factory Add the Zipkin exporter configuration factory using the new configuration format. This does not bring many of the settings from the old configuration since they won't be used. In a sub-sequent PR the code of the exporter itself will be changed. * PR Feedback: add 1 test plus some comments * Rename endpoint to http-url and related field * Fix goimports issue * Remove TODO commented code Replaced the TODO commented code with an issue. * Rename the field used to specify destination * Update README to address reviewer comments. * Update module dependencies and import order for metricfamily * Improve OC Receiver Documentation (#203) * First round of receiver and opencensus receiver documentation. * Undo go mod/sum changes * Address initial set of comments. * Address next set of comments. * Address next set of comments. * Fix use of server instead of receiver in comment and explain settings can be mix and matched. * Merged master and fixed mispell error caugh with new tools * Add static check and fix all errors (#218) * Add static check Fixes #155. * Fix most staticcheck errors * More fixes * Fix id_batcher * Rename exporterhelp parameter (#220) The paramater was named exportFormat but it actually used as the exporter name. * Fix build: lower casing error message (#224) * Add jaeger grpc exporter (#219) * Add Jaeger gRPC exporter Adds a Jaeger gRPC exporter. * Update exporters/README.md * Fixes on the exporter/README.md * Add doc.go with package description. * Fix import order on config_test.go * Migrate updates to Prometheus receiver from opencensus-service * Remove unnecessary configuration for adjusting metrics * Update README to address reviewer comments. * Update module dependencies and import order for metricfamily * Fix goimports * Fix staticcheck issues

…elemetry#219)

…y#219) Bumps [boto3](https://github.com/boto/boto3) from 1.16.43 to 1.17.40. - [Release notes](https://github.com/boto/boto3/releases) - [Changelog](https://github.com/boto/boto3/blob/develop/CHANGELOG.rst) - [Commits](boto/boto3@1.16.43...1.17.40) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

pjanotti requested review from bogdandrutu, flands, songy23 and tigrannajaryan as code owners August 1, 2019 17:42

pjanotti self-assigned this Aug 1, 2019

pjanotti commented Aug 1, 2019

View reviewed changes

songy23 reviewed Aug 1, 2019

View reviewed changes

exporter/jaeger/jaegergrpcexporter/config.go Outdated Show resolved Hide resolved

pjanotti force-pushed the add-jaeger-grpc-exporter branch from 195df26 to 507818a Compare August 1, 2019 18:48

songy23 approved these changes Aug 1, 2019

View reviewed changes

pjanotti mentioned this pull request Aug 2, 2019

Common code for gRPC configuration settings #222

Closed

tigrannajaryan approved these changes Aug 2, 2019

View reviewed changes

Paulo Janotti added 5 commits August 2, 2019 10:35

Add Jaeger gRPC exporter

ed70d6c

Adds a Jaeger gRPC exporter.

Update exporters/README.md

5b4cd20

Fixes on the exporter/README.md

80fb5d8

Add doc.go with package description.

c982293

Fix import order on config_test.go

f3a81ef

pjanotti force-pushed the add-jaeger-grpc-exporter branch from bc4307e to f3a81ef Compare August 2, 2019 17:50

pjanotti merged commit b39842b into open-telemetry:master Aug 2, 2019

pjanotti deleted the add-jaeger-grpc-exporter branch August 2, 2019 18:20

MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021

exporter(jaeger): change jaeger process tags to core.KeyValue (open-t…

11bdacf

…elemetry#219)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add jaeger grpc exporter #219

Add jaeger grpc exporter #219

pjanotti commented Aug 1, 2019

pjanotti Aug 1, 2019

pjanotti Aug 1, 2019

codecov-io commented Aug 1, 2019 •

edited

Loading

songy23 left a comment

songy23 Aug 1, 2019

pjanotti Aug 2, 2019

songy23 Aug 1, 2019

pjanotti Aug 2, 2019

pjanotti Aug 2, 2019

tigrannajaryan Aug 2, 2019

pjanotti Aug 2, 2019


		"go.uber.org/zap"

		"github.com/open-telemetry/opentelemetry-service/config"

Add jaeger grpc exporter #219

Add jaeger grpc exporter #219

Conversation

pjanotti commented Aug 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 1, 2019 • edited Loading

Codecov Report

songy23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 1, 2019 •

edited

Loading