Added Jaeger gRPC receiver #197

jpkrohling · 2019-07-24T14:43:37Z

Closes #127 by implementing a gRPC receiver based on the protobuf specification from Jaeger's own repository.

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

jpkrohling · 2019-07-24T14:45:41Z

Things to discuss:

Should we provide a TLSConfig for the gRPC server? If so, is it better to be part of a follow-up PR?
Related to the test failure: what's an appropriate solution for the lack of link from the parent to child spans?

receiver/jaegerreceiver/trace_receiver.go

receiver/jaegerreceiver/trace_receiver_test.go

translator/trace/jaeger/jaegerproto_to_protospan.go

translator/trace/jaeger/jaegerproto_to_protospan_test.go

jpkrohling · 2019-07-24T15:09:54Z

Travis is failing due to a test failure (see review comment).

receiver/jaegerreceiver/trace_receiver.go

receiver/jaegerreceiver/trace_receiver_test.go

translator/trace/jaeger/jaegerproto_to_protospan_test.go

translator/trace/jaeger/jaegerproto_to_protospan.go

jpkrohling · 2019-07-26T12:26:22Z

This PR has been updated with the results of the first review.

There are still a couple of points to clarify before I can change this PR from its current "draft" status, namely:

Mismatch between Jaeger and OpenCensus/OpenTelemetry when it comes to Links: in Jaeger, we have only a SpanReference on the child span, pointing to its parent. Apparently, OpenTelemetry expects the parent to have a Link to all its children.
Whether span.kind should be removed from the attributes, or kept there duplicating what's already in the property Kind. Other translators seem to duplicate this data, but that doesn't seem appropriate to me.

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

codecov-io · 2019-07-30T08:12:10Z

Codecov Report

Merging #197 into master will increase coverage by 0.44%.
The diff coverage is 85.02%.

@@            Coverage Diff            @@
##           master    #197      +/-   ##
=========================================
+ Coverage   72.75%   73.2%   +0.44%     
=========================================
  Files         107     108       +1     
  Lines        6108    6322     +214     
=========================================
+ Hits         4444    4628     +184     
- Misses       1438    1459      +21     
- Partials      226     235       +9

Impacted Files	Coverage Δ
receiver/jaegerreceiver/factory.go	`96.77% <100%> (+0.94%)`	⬆️
translator/trace/jaeger/status_code.go	`67.74% <61.53%> (-4.49%)`	⬇️
receiver/jaegerreceiver/trace_receiver.go	`77.38% <61.9%> (-2.14%)`	⬇️
...ranslator/trace/jaeger/jaegerproto_to_protospan.go	`91.55% <91.55%> (ø)`
service/service.go	`68.11% <0%> (-0.73%)`	⬇️

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 5f88de9...725556c. Read the comment docs.

jpkrohling · 2019-07-30T08:13:06Z

This is now ready for the final review/merge.

pjanotti

Looks good @jpkrohling - thanks for this PR!

receiver/jaegerreceiver/trace_receiver.go

receiver/jaegerreceiver/trace_receiver_test.go

receiver/jaegerreceiver/factory.go

pjanotti · 2019-07-30T22:13:38Z

Should we provide a TLSConfig for the gRPC server? If so, is it better to be part of a follow-up PR?

We have added TLSConfig to OC gRPC receiver that said I haven't used but others may have.

Is TLS commonly used by Jaeger gRPC? If not I will leave it out (at least in the short run). Anyway, a separate PR will be better for it. (TLS for OC related issue #170)

jpkrohling · 2019-07-31T08:48:00Z

Is TLS commonly used by Jaeger gRPC?

I don't think it's that widespread yet, but we did hear requests about it in the past. In fact, we added support for TLSConfig because of user requests. That said, I think it does make sense to deal with that in a broader way, as the same cert would probably be used for all TLS-capable ports.

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

pjanotti · 2019-07-31T15:52:34Z

@jpkrohling there is one test failure remaining:

--- FAIL: TestCreateWithoutThrift (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x10768dd]

goroutine 27 [running]:
testing.tRunner.func1(0xc000301200)
	/home/travis/.gimme/versions/go1.12.7.linux.amd64/src/testing/testing.go:830 +0x69d
panic(0x117e640, 0x1bef840)
	/home/travis/.gimme/versions/go1.12.7.linux.amd64/src/runtime/panic.go:522 +0x1b5
github.com/open-telemetry/opentelemetry-service/receiver/jaegerreceiver.(*Factory).CreateTraceReceiver(0xc000068ed8, 0x1420580, 0xc00003a1d8, 0xc000068f08, 0x1423900, 0xc00029f6e0, 0x0, 0x0, 0xc0002e14a0, 0x58150b, ...)
	/home/travis/gopath/src/github.com/open-telemetry/opentelemetry-service/receiver/jaegerreceiver/factory.go:137 +0x4dd
github.com/open-telemetry/opentelemetry-service/receiver/jaegerreceiver.TestCreateWithoutThrift(0xc000301200)
	/home/travis/gopath/src/github.com/open-telemetry/opentelemetry-service/receiver/jaegerreceiver/factory_test.go:152 +0x248
testing.tRunner(0xc000301200, 0x12cc228)
	/home/travis/.gimme/versions/go1.12.7.linux.amd64/src/testing/testing.go:865 +0x164
created by testing.(*T).Run
	/home/travis/.gimme/versions/go1.12.7.linux.amd64/src/testing/testing.go:916 +0x65b
FAIL	github.com/open-telemetry/opentelemetry-service/receiver/jaegerreceiver	0.054s

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

jpkrohling · 2019-07-31T16:07:18Z

there is one test failure remaining

Sorry, this was caused by the check I added in a previous commit, validating that the ip:ports are unique. I'm removing this feature, as this will fail soon after that anyway, and the only value-add we are offering is a better error message.

* 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 <juraci@kroehling.de> * Addressed first review round Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Changes based on the feedback from the second review Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Fixed import order, check for same ip:port on endpoints Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Reverted the port-check Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Fix README - remove broken links and commands (#211) Signed-off-by: Hui Kang <kangh@us.ibm.com> * 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

* Make span start/end configuration more greppable Rename SpanOption to StartOption Rename StartOptions to StartConfig Rename EndOptions to EndConfig fixes open-telemetry#197

jpkrohling commented Jul 24, 2019

View reviewed changes

receiver/jaegerreceiver/trace_receiver.go Show resolved Hide resolved

jpkrohling commented Jul 24, 2019

View reviewed changes

pjanotti reviewed Jul 25, 2019

View reviewed changes

jpkrohling added 2 commits July 30, 2019 09:25

Added Jaeger gRPC receiver

48f7c18

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

Addressed first review round

3e916be

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

jpkrohling mentioned this pull request Jul 30, 2019

Links from parent to child not supported in Jaeger #206

Closed

2 tasks

jpkrohling force-pushed the 127-Added-jaeger-grpc-receiver branch from aa8c160 to 7462a8f Compare July 30, 2019 07:40

Changes based on the feedback from the second review

88f8ca9

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

jpkrohling force-pushed the 127-Added-jaeger-grpc-receiver branch from 7462a8f to 88f8ca9 Compare July 30, 2019 07:59

jpkrohling marked this pull request as ready for review July 30, 2019 08:12

jpkrohling requested review from bogdandrutu, flands, songy23 and tigrannajaryan as code owners July 30, 2019 08:12

pjanotti approved these changes Jul 30, 2019

View reviewed changes

receiver/jaegerreceiver/trace_receiver.go Show resolved Hide resolved

receiver/jaegerreceiver/trace_receiver_test.go Outdated Show resolved Hide resolved

songy23 approved these changes Jul 30, 2019

View reviewed changes

receiver/jaegerreceiver/factory.go Show resolved Hide resolved

songy23 assigned jpkrohling Jul 30, 2019

Fixed import order, check for same ip:port on endpoints

62bb5e6

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

Reverted the port-check

725556c

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

pjanotti merged commit 1969524 into open-telemetry:master Jul 31, 2019

objectiser mentioned this pull request Feb 28, 2020

TLS config for Jaeger gRPC receiver jaegertracing/jaeger-opentelemetry-collector#22

Closed

hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023

Add UF doc (open-telemetry#197)

52d34d2

Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024

Support links and event attributes in recordable (open-telemetry#197)

eb7cbd1

swiatekm pushed a commit to swiatekm/opentelemetry-collector that referenced this pull request Oct 9, 2024

Support pod lifecycle hooks (open-telemetry#197)

dea7c76

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Jaeger gRPC receiver #197

Added Jaeger gRPC receiver #197

jpkrohling commented Jul 24, 2019 •

edited

Loading

jpkrohling commented Jul 24, 2019 •

edited

Loading

jpkrohling commented Jul 24, 2019

jpkrohling commented Jul 26, 2019

codecov-io commented Jul 30, 2019 •

edited

Loading

jpkrohling commented Jul 30, 2019

pjanotti left a comment

pjanotti commented Jul 30, 2019

jpkrohling commented Jul 31, 2019 •

edited

Loading

pjanotti commented Jul 31, 2019

jpkrohling commented Jul 31, 2019

Added Jaeger gRPC receiver #197

Added Jaeger gRPC receiver #197

Conversation

jpkrohling commented Jul 24, 2019 • edited Loading

jpkrohling commented Jul 24, 2019 • edited Loading

jpkrohling commented Jul 24, 2019

jpkrohling commented Jul 26, 2019

codecov-io commented Jul 30, 2019 • edited Loading

Codecov Report

jpkrohling commented Jul 30, 2019

pjanotti left a comment

Choose a reason for hiding this comment

pjanotti commented Jul 30, 2019

jpkrohling commented Jul 31, 2019 • edited Loading

pjanotti commented Jul 31, 2019

jpkrohling commented Jul 31, 2019

jpkrohling commented Jul 24, 2019 •

edited

Loading

jpkrohling commented Jul 24, 2019 •

edited

Loading

codecov-io commented Jul 30, 2019 •

edited

Loading

jpkrohling commented Jul 31, 2019 •

edited

Loading