Add all-in-one OTEL component #2262

pavolloffay · 2020-05-28T13:11:48Z

Resolves #2258
Resolves #2159

Add all-in-one based on OpenTelemetry collector

Notable changes:

OTEL in-memory exporter - jaeger_memory
query service is configurable only via flags
archive storage in query is configurable only via flags
removed dependency of admin service from query. Now the query reports HC status changes via channel
refactored default configuration

Follow up issues/PR:

expose OTEL logger Expose service's logger open-telemetry/opentelemetry-collector#1032
subscribe to OTEL lifecycle events Subscribe to shutdown event open-telemetry/opentelemetry-collector#1033

objectiser

Interim review - will continue after current call.

Makefile

cmd/all-in-one/main.go

objectiser · 2020-05-28T13:51:47Z

cmd/opentelemetry-collector/app/defaults/default_config.go

-	jaeger := factories.Receivers["jaeger"].CreateDefaultConfig().(*jaegerreceiver.Config)
-	if jaeger.Protocols == nil {
-		jaeger.Protocols = map[string]*receiver.SecureReceiverSettings{}
+func createReceivers(component ComponentType, zipkinHostPort string, factories config.Factories) configmodels.Receivers {


This could be func (c ComponentSettings) createReceivers() to avoid passing params? Same for createExporters.

I have done this on purpose - be explicit on the required API contract.

The public API is easy to use. I like to keep the internal APIs clear in terms of the required arguments.

objectiser · 2020-05-28T14:03:43Z

cmd/opentelemetry-collector/app/defaults/default_config_test.go

 					},
 				},
 			},
 		},
 		{
-			args: []string{
-				"--processor.jaeger-binary.server-host-port=host:1",


Looks like this test has been removed? I think this is still required.

The code is fully covered in this class. The same test is in jaeger_receiver_test.go.

objectiser · 2020-05-28T14:08:27Z

cmd/opentelemetry-collector/app/defaults/default_config_test.go

 	"go.uber.org/zap"

 	"github.com/jaegertracing/jaeger/cmd/opentelemetry-collector/app"
 	"github.com/jaegertracing/jaeger/cmd/opentelemetry-collector/app/exporter/cassandra"
 	"github.com/jaegertracing/jaeger/cmd/opentelemetry-collector/app/exporter/elasticsearch"
 	"github.com/jaegertracing/jaeger/cmd/opentelemetry-collector/app/exporter/grpcplugin"
-	"github.com/jaegertracing/jaeger/cmd/opentelemetry-collector/app/exporter/kafka"


Would be worth adding kafka exporter in one of the tests, just for coverage.

It's already there but was using a string, I will change it to use the constant.

cmd/opentelemetry-collector/app/defaults/defaults.go

cmd/opentelemetry-collector/app/exporter/cassandra/factory.go

cmd/opentelemetry-collector/app/exporter/elasticsearch/config_test.go

cmd/opentelemetry-collector/app/exporter/elasticsearch/factory.go

cmd/opentelemetry-collector/app/exporter/memory/factory.go

cmd/opentelemetry-collector/cmd/all-in-one/main.go

cmd/opentelemetry-collector/app/exporter/memory/factory_test.go

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay · 2020-05-29T16:01:38Z

@objectiser I have removed the archive storage config from exporters. It looks cleaner now.

pavolloffay · 2020-05-29T16:01:50Z

It should be ready for another review.

objectiser

A couple of comments - but looks like an issue in CI.

cmd/opentelemetry-collector/cmd/all-in-one/main.go

objectiser · 2020-06-01T17:03:33Z

cmd/opentelemetry-collector/cmd/all-in-one/main.go

+		}
+		return f, nil
+	case "memory":
+		return memory.GetFactory(), nil


Why not badger, grpc_plugin?

badger hasn't been implemented yet. I will add the grpc-plugin.

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

codecov · 2020-06-02T11:31:10Z

Codecov Report

Merging #2262 into master will increase coverage by 0.02%.
The diff coverage is 96.07%.

@@            Coverage Diff             @@
##           master    #2262      +/-   ##
==========================================
+ Coverage   96.14%   96.17%   +0.02%     
==========================================
  Files         219      219              
  Lines       10658    10686      +28     
==========================================
+ Hits        10247    10277      +30     
+ Misses        354      353       -1     
+ Partials       57       56       -1

Impacted Files	Coverage Δ
plugin/storage/es/factory.go	`100.00% <ø> (ø)`
cmd/query/app/server.go	`94.80% <90.00%> (+0.21%)`	⬆️
plugin/storage/cassandra/options.go	`100.00% <100.00%> (ø)`
plugin/storage/es/options.go	`100.00% <100.00%> (ø)`
plugin/storage/memory/factory.go	`100.00% <100.00%> (ø)`
plugin/storage/memory/options.go	`100.00% <100.00%> (ø)`
cmd/query/app/static_handler.go	`81.66% <0.00%> (-2.50%)`	⬇️
cmd/flags/admin.go	`79.36% <0.00%> (+1.58%)`	⬆️
pkg/netutils/port.go	`100.00% <0.00%> (+50.00%)`	⬆️

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 2fdd1cd...d21c888. Read the comment docs.

objectiser

LGTM - once CI passed. Maybe get Yuri to check this one?

cmd/opentelemetry-collector/go.sum

cmd/opentelemetry-collector/cmd/all-in-one/main.go

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay · 2020-06-03T12:03:19Z

The last fixes building OTEL all-in-one image and runs integration test for it.

I will merge on green.

RehanSaeed · 2020-12-12T12:16:22Z

Is there any documentation regarding this Docker image? In particular, which ports I need. The getting started guide seems to cover the non-open telemetry version https://www.jaegertracing.io/docs/1.21/getting-started/.

objectiser · 2020-12-14T11:03:57Z

@RehanSaeed The information is contained in this section: https://www.jaegertracing.io/docs/1.21/opentelemetry/

pavolloffay requested a review from objectiser May 28, 2020 13:11

pavolloffay requested a review from a team as a code owner May 28, 2020 13:11

objectiser reviewed May 28, 2020

View reviewed changes

Add all-in-one OTEL component

6d4b426

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay force-pushed the otel-all-in-one branch from c6f6cf4 to 6d4b426 Compare May 29, 2020 09:34

pavolloffay added 6 commits May 29, 2020 11:53

Use logger

b26f931

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Do not populate archive storage to OTEL exporters

ac4c7ac

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Increase test coverage

87d4121

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Use older OTEL version to avoid internal packages

e985acd

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Cleanup

37f4043

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Refactor

7853fcc

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

objectiser reviewed Jun 1, 2020

View reviewed changes

pavolloffay added 2 commits June 2, 2020 13:03

Bump otel and use status channel

dae44ce

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

add grpc plugin

c9521d9

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay mentioned this pull request Jun 2, 2020

Add Badger OTEL exporter #2269

Merged

pavolloffay added the area/otel label Jun 2, 2020

objectiser approved these changes Jun 2, 2020

View reviewed changes

cmd/opentelemetry-collector/go.sum Show resolved Hide resolved

cmd/opentelemetry-collector/cmd/all-in-one/main.go Outdated Show resolved Hide resolved

pavolloffay added 2 commits June 2, 2020 18:10

tidy and grpc plugin

1ca45ec

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Run itest for OTEL allinone

d21c888

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay merged commit 6f3a169 into jaegertracing:master Jun 3, 2020

albertteoh mentioned this pull request Mar 5, 2021

Remove unused NewOptionsFromConfig #2863

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add all-in-one OTEL component #2262

Add all-in-one OTEL component #2262

pavolloffay commented May 28, 2020 •

edited

Loading

objectiser left a comment

objectiser May 28, 2020

pavolloffay May 28, 2020

pavolloffay May 28, 2020

objectiser May 28, 2020

pavolloffay May 28, 2020

objectiser May 28, 2020

pavolloffay May 28, 2020

pavolloffay commented May 29, 2020

pavolloffay commented May 29, 2020

objectiser left a comment

objectiser Jun 1, 2020

pavolloffay Jun 2, 2020

codecov bot commented Jun 2, 2020 •

edited

Loading

objectiser left a comment

pavolloffay commented Jun 3, 2020

RehanSaeed commented Dec 12, 2020 •

edited

Loading

objectiser commented Dec 14, 2020

Add all-in-one OTEL component #2262

Add all-in-one OTEL component #2262

Conversation

pavolloffay commented May 28, 2020 • edited Loading

objectiser 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

pavolloffay commented May 29, 2020

pavolloffay commented May 29, 2020

objectiser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 2, 2020 • edited Loading

Codecov Report

objectiser left a comment

Choose a reason for hiding this comment

pavolloffay commented Jun 3, 2020

RehanSaeed commented Dec 12, 2020 • edited Loading

objectiser commented Dec 14, 2020

pavolloffay commented May 28, 2020 •

edited

Loading

codecov bot commented Jun 2, 2020 •

edited

Loading

RehanSaeed commented Dec 12, 2020 •

edited

Loading