Introduce OpenTelemetry collector #2086

pavolloffay · 2020-02-25T15:59:52Z

Fixes jaegertracing/jaeger-opentelemetry-collector#8

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

pavolloffay · 2020-02-25T16:05:43Z

cmd/opentelemetry-collector/main.go

+
+	// TODO caused compilation issues due to refactoring in https://github.com/jaegertracing/jaeger/pull/2060
+	// TODO app/HandlerConfig moved to app/handler/HandlerConfig
+	//cmpts, err := defaults.Components()


I could not initialize all default components (importers, processors, exporters) because Jaeger importer imports jaeger/collector/app and there was a breaking change #2060

Once open-telemetry/opentelemetry-collector#570 is merged we should be able to use defaults.components()

I commented on open-telemetry/opentelemetry-collector#570 and we can merge it quickly if it blocks you.

yurishkuro · 2020-02-25T16:12:47Z

cmd/opentelemetry-collector/main.go

+package main
+
+import (
+	"github.com/open-telemetry/opentelemetry-collector/config"


there should be a corresponding change in Gopkg.toml

pavolloffay · 2020-02-25T20:45:55Z

So importing opentelemetry-collector will certainly introduce some complications.

For instance a breaking change in this repo might result in a compilation error (like in https://github.com/jaegertracing/jaeger/pull/2086/files/de7040ace7ce90518b1ff99c32f471a801d6665c#diff-bc7bd0edae17579706fb28fa796dd8cf).

The opposite approach might be to move Jaeger components here and import them in opentelemetry-collector if we assume that OTEL interfaces will not break (I assume they will break less than Jaeger code).

An alternative solution might be to re-implement jaeger components directly here and use those instead. We could just import a shared Jaeger configuration from OTEL (It would have to be moved to a separate package). I frankly don't like this option.

Also Jaeger uses go dep and OTEL go modules. I am not sure if this will cause any problems once we switch Jaeger to go modules #1546.

Any thoughts @yurishkuro @tigrannajaryan?

tigrannajaryan · 2020-02-26T01:09:03Z

So importing opentelemetry-collector will certainly introduce some complications.

For instance a breaking change in this repo might result in a compilation error (like in https://github.com/jaegertracing/jaeger/pull/2086/files/de7040ace7ce90518b1ff99c32f471a801d6665c#diff-bc7bd0edae17579706fb28fa796dd8cf).

The opposite approach might be to move Jaeger components here and import them in opentelemetry-collector if we assume that OTEL interfaces will not break (I assume they will break less than Jaeger code).

This would mean this repo references opentelemetry-collector and opentelemetry-collector references this repo. I do not know if go modules allow a circular references between repos, but even if it does it will not look nice. I'd rather not do it.

Alternatively we could references Jaeger components located in this repo from opentelemetry-collector-contrib. That would no longer be a circular reference, however it would put Jaeger out of core build, which I would not want to do (among other things we committed to having built-in support for Jaeger in Collector core).

An alternative solution might be to re-implement jaeger components directly here and use those instead. We could just import a shared Jaeger configuration from OTEL (It would have to be moved to a separate package). I frankly don't like this option.

Yeah, this does not sound like a good solution. Lots of code duplication.

Also Jaeger uses go dep and OTEL go modules. I am not sure if this will cause any problems once we switch Jaeger to go modules #1546.

I have no experience mixing the 2 approaches.

yurishkuro · 2020-02-26T02:03:59Z

@pavolloffay can you enumerate which parts of Jaeger the OTel Collector imports?

pavolloffay · 2020-02-26T10:01:59Z

// exporter, receiver
        "github.com/jaegertracing/jaeger/proto-gen/api_v2"
// receiver, translator
	"github.com/jaegertracing/jaeger/thrift-gen/baggage"
	"github.com/jaegertracing/jaeger/thrift-gen/jaeger"
	"github.com/jaegertracing/jaeger/thrift-gen/jaeger"
	"github.com/jaegertracing/jaeger/thrift-gen/sampling"
	"github.com/jaegertracing/jaeger/thrift-gen/zipkincore"

// receiver
	"github.com/jaegertracing/jaeger/cmd/agent/app/configmanager"
	"github.com/jaegertracing/jaeger/cmd/agent/app/configmanager/grpc"
	"github.com/jaegertracing/jaeger/cmd/agent/app/httpserver"
	"github.com/jaegertracing/jaeger/cmd/agent/app/processors"
	"github.com/jaegertracing/jaeger/cmd/agent/app/reporter"
	"github.com/jaegertracing/jaeger/cmd/agent/app/servers"
	"github.com/jaegertracing/jaeger/cmd/agent/app/servers/thriftudp"
	"github.com/jaegertracing/jaeger/cmd/collector/app/handler"
	"github.com/jaegertracing/jaeger/cmd/collector/app/sampling"
	"github.com/jaegertracing/jaeger/plugin/sampling/strategystore/static"

// translator
	"github.com/jaegertracing/jaeger/model"

annanay25 · 2020-02-26T11:59:33Z

Is it possible to extract the common jaeger components into packages, say at github.com/jaegertracing/jaeger/pkg and have both Jaeger and OTel import these?

yurishkuro · 2020-02-26T14:44:47Z

It is possible, but I don’t like it. It will make jaeger development harder by moving critical parts to another repo.

pavolloffay · 2020-02-26T16:00:47Z

This would mean this repo references opentelemetry-collector and opentelemetry-collector references this repo. I do not know if go modules allow a circular references between repos, but even if it does it will not look nice. I'd rather not do it.

One way or another there will be a cyclic dependency if we want to have the jaeger-opentelemetry-collector in this repository.
I don't know how it exactly works, maybe the cycle is not allowed only per packages and can be used per repositories.

I think it will be better to host as much as possible Jaeger related code in Jaeger repository. We could have OTEL importer, processor, exporter like API implementation in Jaeger (not direct import of OTEL). Then the actual Jaeger importer, processor, exporter will use this package to implement OTEL components. This way if we break API in Jaeger it will not result in a compilation error like it did now. OpenTelemetry will be able to break API too as the jaeger importers, processors will be implemented directly in the OTEL.

yurishkuro · 2020-02-26T16:14:18Z

@pavolloffay maybe it’s worth trying these cross-repo imports on a couple toy projects using go mod.

pavolloffay · 2020-02-26T16:16:08Z

If we agree that Jaeger can be moved to go mod I can try directly with Jaeger and OTEL. Actually I have started some work already for #2019.

tigrannajaryan · 2020-02-26T17:52:23Z

One way or another there will be a cyclic dependency if we want to have the jaeger-opentelemetry-collector in this repository.
I don't know how it exactly works, maybe the cycle is not allowed only per packages and can be used per repositories.

If you use go.mod then every go.mod containing directory is considered a separate module (minus subdirectories that contain their own go.mod files). So even though it is a circular dependency between repos, it will no longer be considered a circular dependency between go modules if you structure them appropriately (and I think go tooling should be fine with it, but don't trust my word, I never tried this actual setting).

I think it will be better to host as much as possible Jaeger related code in Jaeger repository. We could have OTEL importer, processor, exporter like API implementation in Jaeger (not direct import of OTEL). Then the actual Jaeger importer, processor, exporter will use this package to implement OTEL components. This way if we break API in Jaeger it will not result in a compilation error like it did now. OpenTelemetry will be able to break API too as the jaeger importers, processors will be implemented directly in the OTEL.

Makes sense.

pavolloffay · 2020-02-28T13:49:37Z

First we should migrate to go modules #2098.

Based on this PR I have tried importing Jaeger in OTEL via override, the build worked and I was able to build Jaeger with imported OTEL (This is the option 2).

Then there are two approaches we can take:

define a separate module for jaeger-otel-collector. This module will import Jaeger and OTEL. So if there are breaking changes in Jaeger we will be able to coordinate fix in OTEL and subsequently build jaeger-otel collector with bumped versions of OTEL and jaeger.
Do not define go module for jaeger-otelcollector and just simply import OTEL in jaeger. This is more dangerous and we could run into build problems. These problems could be minimized by moving most of the Jaeger code from OTEL to Jaeger. Then the Jaeger and OTEL would import this code and use it to implement importers, processors, exporters.

Option 1. seems like a better option to` me. Although, I found several comments on github discouraging this approach especially when the module should serve as a library or API, but we are just trying to use it for the main package mainly. I didn't find any exact reasons why. I will have to experiment more with this.

https://github.com/go-modules-by-example/index/blob/master/009_submodules/README.md
Here are some references where go submodules have been discussed
https://groups.google.com/forum/#!msg/prometheus-developers/F1Vp0rLk3TQ/F412gRw0CAAJ
Consider making the pkg/apis/monitoring package a nested go module prometheus-operator/prometheus-operator#2988 (comment)
cmd/go: setting up a multi-module single repo is difficult golang/go#27056

yurishkuro · 2020-02-28T17:24:34Z

define a separate module for jaeger-otel-collector

what do you mean by that?

pavolloffay · 2020-03-02T09:02:14Z

Use go submodules. Multiple go modules in a single repository.

pavolloffay · 2020-03-02T16:37:58Z

cmd/opentelemetry-collector/go.mod

@@ -0,0 +1,9 @@
+module github.com/jaegertracing/jaeger/cmd/opentelemetry-collector


@yurishkuro this is an example fo go submodule.

By default Jaeger version would be resolved from OTEL collector, but with replace we are able to point it to the current master/head. If we do a breaking change we can pin the version to a version prior the change and collaborate with OTEL to update the code there.

pavolloffay · 2020-03-19T11:04:59Z

@yurishkuro we should make progress on this one. The work in https://github.com/jaegertracing/jaeger-opentelemetry-collector will require changes in the core. For instance changing storage writers to work on the batches rather than on a single spans https://github.com/jaegertracing/jaeger-opentelemetry-collector/issues/15.

yurishkuro · 2020-03-19T14:58:00Z

is there an open question?

pavolloffay · 2020-03-19T15:17:33Z

There aren't any open questions. It just needs to be merged if we agree that this can go in.

A separate go module for the otel collector is the safest path for now. There are still breaking changes happening if there wasn't we could import it in the root package.

There was a discussion regarding the versioning. The jaeger-otel collector will follow the version schema for with jaeger binaries, so with regards to this point it is also fine.

codecov · 2020-03-19T17:05:21Z

Codecov Report

Merging #2086 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2086   +/-   ##
=======================================
  Coverage   96.13%   96.13%           
=======================================
  Files         218      218           
  Lines       10564    10564           
=======================================
  Hits        10156    10156           
  Misses        353      353           
  Partials       55       55

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 83b2679...b36d39c. Read the comment docs.

yurishkuro · 2020-03-20T17:02:08Z

cmd/opentelemetry-collector/main.go

@@ -0,0 +1,50 @@
+// Copyright (c) 2020 The Jaeger Authors.


what is the binary file above? probably don't want it checked in.

yurishkuro · 2020-03-20T17:05:08Z

.travis.yml

@@ -36,6 +36,9 @@ matrix:
  - go: "1.13.x"
    env:
    - HOTROD=true
+  - go: "1.13.x"


Why do we need a separate matrix step for it? I'd rather include it in the main build.

+1 this was just to quickly test the build. I will remove it

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

pavolloffay requested a review from a team as a code owner February 25, 2020 15:59

pavolloffay requested a review from jpkrohling February 25, 2020 15:59

pavolloffay commented Feb 25, 2020

View reviewed changes

yurishkuro reviewed Feb 25, 2020

View reviewed changes

pavolloffay force-pushed the import-opentelemetry branch from 5332679 to a17bab7 Compare March 2, 2020 16:34

pavolloffay commented Mar 2, 2020

View reviewed changes

pavolloffay force-pushed the import-opentelemetry branch 3 times, most recently from 9e3ac6a to 02304e7 Compare March 19, 2020 11:00

pavolloffay changed the title ~~WIP: Intoduce OpenTelemetry collector~~ Intoduce OpenTelemetry collector Mar 19, 2020

yurishkuro approved these changes Mar 20, 2020

View reviewed changes

Introduce OTEL collector as go submodule

35974f9

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

pavolloffay force-pushed the import-opentelemetry branch from 2a7ed3d to 35974f9 Compare March 23, 2020 09:18

pavolloffay added 3 commits March 23, 2020 10:19

Remove binary

86ea558

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

Remove otel job from travis

d8d9f99

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

Fix typo

b36d39c

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

pavolloffay changed the title ~~Intoduce OpenTelemetry collector~~ Introduce OpenTelemetry collector Mar 23, 2020

pavolloffay merged commit 87c466d into jaegertracing:master Mar 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce OpenTelemetry collector #2086

Introduce OpenTelemetry collector #2086

pavolloffay commented Feb 25, 2020

pavolloffay Feb 25, 2020

pavolloffay Feb 25, 2020

tigrannajaryan Feb 26, 2020

yurishkuro Feb 25, 2020

pavolloffay commented Feb 25, 2020

tigrannajaryan commented Feb 26, 2020

yurishkuro commented Feb 26, 2020

pavolloffay commented Feb 26, 2020

annanay25 commented Feb 26, 2020

yurishkuro commented Feb 26, 2020

pavolloffay commented Feb 26, 2020

yurishkuro commented Feb 26, 2020

pavolloffay commented Feb 26, 2020

tigrannajaryan commented Feb 26, 2020

pavolloffay commented Feb 28, 2020 •

edited

Loading

yurishkuro commented Feb 28, 2020

pavolloffay commented Mar 2, 2020

pavolloffay Mar 2, 2020

pavolloffay commented Mar 19, 2020

yurishkuro commented Mar 19, 2020

pavolloffay commented Mar 19, 2020

codecov bot commented Mar 19, 2020 •

edited

Loading

yurishkuro Mar 20, 2020

yurishkuro Mar 20, 2020

pavolloffay Mar 20, 2020

		@@ -0,0 +1,9 @@
		module github.com/jaegertracing/jaeger/cmd/opentelemetry-collector

Introduce OpenTelemetry collector #2086

Introduce OpenTelemetry collector #2086

Conversation

pavolloffay commented Feb 25, 2020

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 Feb 25, 2020

tigrannajaryan commented Feb 26, 2020

yurishkuro commented Feb 26, 2020

pavolloffay commented Feb 26, 2020

annanay25 commented Feb 26, 2020

yurishkuro commented Feb 26, 2020

pavolloffay commented Feb 26, 2020

yurishkuro commented Feb 26, 2020

pavolloffay commented Feb 26, 2020

tigrannajaryan commented Feb 26, 2020

pavolloffay commented Feb 28, 2020 • edited Loading

yurishkuro commented Feb 28, 2020

pavolloffay commented Mar 2, 2020

Choose a reason for hiding this comment

pavolloffay commented Mar 19, 2020

yurishkuro commented Mar 19, 2020

pavolloffay commented Mar 19, 2020

codecov bot commented Mar 19, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Feb 28, 2020 •

edited

Loading

codecov bot commented Mar 19, 2020 •

edited

Loading