Merge hardcoded/default configuration with OTEL config file #2211

pavolloffay · 2020-04-29T15:32:14Z

Resolves #2198

This PR changes the way how OTEL config is loaded:

Now when OTEL config is provided it is merged with hardcoded/default Jaeger opinionated config.
hardcoded/default Jaeger components can be disabled in OTEL config via disabled property.

Caveats:

OTEL config.Load() returns an error if config does not contain receivers or exporters. We should fix this in upstream and move all validations to a separate function. - Allow loading incomplete configuration open-telemetry/opentelemetry-collector#888

Signed-off-by: Pavol Loffay <[email protected]>

cmd/opentelemetry-collector/go.mod

pavolloffay · 2020-04-29T15:38:41Z

@objectiser @yurishkuro could you please review?

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay · 2020-04-30T12:58:51Z

The xdock tests are flaky. I think it's because of gRPC channel between agent and collector. I will try to use retry and report back. We had similar issue here https://github.com/jaegertracing/jaeger/pull/1384/files

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay · 2020-04-30T14:19:58Z

This patch in OTEL should fix the failures in OTEL xdoc open-telemetry/opentelemetry-collector#899. We should also apply the same to our current xdock.

Signed-off-by: Pavol Loffay <[email protected]>

codecov · 2020-04-30T14:27:44Z

Codecov Report

Merging #2211 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2211      +/-   ##
==========================================
- Coverage   96.18%   96.16%   -0.02%     
==========================================
  Files         219      219              
  Lines       10632    10632              
==========================================
- Hits        10226    10224       -2     
- Misses        351      352       +1     
- Partials       55       56       +1

Impacted Files	Coverage Δ
cmd/query/app/server.go	`91.78% <0.00%> (-2.74%)`	⬇️

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 f834975...f774bf4. Read the comment docs.

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay · 2020-05-01T08:45:55Z

@rubenvp8510 would you like to review this one?

objectiser · 2020-05-01T13:46:40Z

cmd/opentelemetry-collector/app/defaults/testdata/addprocessor-override.yaml

@@ -0,0 +1,23 @@
+receivers:


Can't the receivers and exporters be removed now?

Not now. There is a comment in this file explaining why.

See also open-telemetry/opentelemetry-collector#888

objectiser · 2020-05-01T13:49:42Z

cmd/opentelemetry-collector/app/defaults/testdata/addprocessor-merged.yaml

+  pipelines:
+    traces:
+      receivers: [jaeger]
+      processors: [queued_retry, batch]


This may highlight a potential issue in some situations - what if a user wants the processors to be performed in a particular order?
In this specific case, wouldn't the batch normally precede the queued_retry? So the spans are batched, and then queued/retried based on those new batches?

This is a good comment. We should allow overriding the order.

In https://github.com/open-telemetry/opentelemetry-collector/pull/909` disabled flag has been removed from all components. Hence there is no way how a user could disable hardcoded/default component.

In this PR I wanted to support incremental addition of components. For instance if a user wants to add a processor it would just add it to services.pipelines.traces.processors without repeating the default/hardcoded processors. Since there is no way to disable a component and order of the components might be important we should require defining an exact order of the components in the pipeline.

Example: add attribute processor

processors: attributes: key: foo service: pipelines: traces: processors: ["batch", "attributes"]

The batch has to be repeated otherwise it will not be included in the pipeline.

Is this going to be considered/implemented in a separate PR?

In this PR, as this will have to change anyways.

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

cmd/opentelemetry-collector/go.sum

objectiser · 2020-05-01T13:59:43Z

cmd/opentelemetry-collector/main.go

+				return nil, err
+			}
+		}
+		err = defaults.MergeConfigs(cfg, otelCfg)


So just to clarify, the user supplied OTel config will override any config provided by jaeger's default OTel config or CLI flags?

OTel config will override config properties that are overridden in OTEL config. This is why we do merge instead of ignoring Jaeger config like we were doing prior this PR.

objectiser · 2020-05-01T14:00:47Z

crossdock/otel-agent-config.yml

@@ -4,15 +4,19 @@ receivers:
      host_endpoint: localhost:5778
      fetch_endpoint: jaeger-collector:14250
    protocols:
+      grpc:


Why has this been added (and disabled) rather than just leaving out?

Because grpc is enabled by default and our legacy agent doesn't expose gRPC endpoint. Once we provide agent build with opinionated config this will be removed.

objectiser · 2020-05-01T14:01:12Z

crossdock/otel-agent-config.yml

      thrift_compact:
      thrift_binary:

 exporters:
+  jaeger_cassandra:


Same as comment above.

Collector config by default uses cassandra backed. In this case we want to disable C* storage because this deployment acts as agent.

Ah ok - but when there is an opinionated default config for agent this won't be a problem, as you explained above.

objectiser · 2020-05-01T14:05:46Z

crossdock/otel-agent-config.yml


 service:
  pipelines:
    traces:
-      receivers: [jaeger]


Why has jaeger receiver been dropped?

Are we also going to support an agent config using otel protocol receiver and exporter config? For example when using an OTel tracer with otel protocol exporter - it might be good to collect via otel agent (no jaeger receiver/exporter) and talk otel protocol to the backend jaeger collector, i.e. so no jaeger protocol used.

Why has jaeger receiver been dropped?

Jaeger receiver is enabled by default, hence we don't have to specify it explicitly. In this config file we just disabled the grpc endpoint - to better align with legacy agent capabilities. Once we create agent OTEL build this config file will change bc the agent will use a different hardcoded default config #2204

Are we also going to support an agent config using otel protocol receiver and exporter config? For example when using an OTel tracer with otel protocol exporter - it might be good to collect via otel agent (no jaeger receiver/exporter) and talk otel protocol to the backend jaeger collector, i.e. so no jaeger protocol used.

This is a separate discussion but I agree, we should be enabling OTEL endpoints by default. #2214

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

rubenvp8510 · 2020-05-01T16:38:21Z

cmd/opentelemetry-collector/main.go

+		if len(getOTELConfigFile()) > 0 {
+			otelCfg, err = service.FileLoaderConfigFactory(otelViper, f)
+			if err != nil {
+				return nil, err


Should we log an error here? Indicating a config load failure?

It's not needed here as It is logged in OTEL collector.

Signed-off-by: Pavol Loffay <[email protected]>

objectiser

Just one minor thing.

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

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay added 2 commits April 29, 2020 17:16

Merge hardcoded/default configuration with OTEL config file

8da977d

Signed-off-by: Pavol Loffay <[email protected]>

cleanup

0d0230e

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay added the area/otel label Apr 29, 2020

pavolloffay requested a review from a team as a code owner April 29, 2020 15:32

pavolloffay requested a review from jpkrohling April 29, 2020 15:32

pavolloffay commented Apr 29, 2020

View reviewed changes

cmd/opentelemetry-collector/go.mod Outdated Show resolved Hide resolved

pavolloffay mentioned this pull request Apr 29, 2020

Allow loading incomplete configuration open-telemetry/opentelemetry-collector#888

Closed

pavolloffay added 4 commits April 30, 2020 09:16

Copy the func for now

22f38a9

Signed-off-by: Pavol Loffay <[email protected]>

disable c* exporter

7e7344a

Signed-off-by: Pavol Loffay <[email protected]>

disable grpc

472bd45

Signed-off-by: Pavol Loffay <[email protected]>

Try depends on

f076773

Signed-off-by: Pavol Loffay <[email protected]>

Use OTEL with wait for ready

13940e4

Signed-off-by: Pavol Loffay <[email protected]>

cleanup

ae5bbe4

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay mentioned this pull request Apr 30, 2020

Set wait for ready in Jaeger gRPC exporter open-telemetry/opentelemetry-collector#899

Merged

Bump otel version that has all fixes

5c7e93d

Signed-off-by: Pavol Loffay <[email protected]>

objectiser reviewed May 1, 2020

View reviewed changes

rubenvp8510 reviewed May 1, 2020

View reviewed changes

cmd/opentelemetry-collector/app/defaults/default_config_test.go Outdated Show resolved Hide resolved

rubenvp8510 reviewed May 1, 2020

View reviewed changes

objectiser approved these changes May 4, 2020

View reviewed changes

pavolloffay added 3 commits May 4, 2020 11:35

Bump OTEL version and remove disabled flag

f1f1a4a

Signed-off-by: Pavol Loffay <[email protected]>

change import

f839761

Signed-off-by: Pavol Loffay <[email protected]>

Always override arrays

2d75d93

Signed-off-by: Pavol Loffay <[email protected]>

objectiser approved these changes May 4, 2020

View reviewed changes

cmd/opentelemetry-collector/app/defaults/merge_test.go Outdated Show resolved Hide resolved

Remove comment

f774bf4

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay merged commit e43a11d into jaegertracing:master May 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge hardcoded/default configuration with OTEL config file #2211

Merge hardcoded/default configuration with OTEL config file #2211

pavolloffay commented Apr 29, 2020 •

edited

Loading

pavolloffay commented Apr 29, 2020

pavolloffay commented Apr 30, 2020

pavolloffay commented Apr 30, 2020

codecov bot commented Apr 30, 2020 •

edited

Loading

pavolloffay commented May 1, 2020

objectiser May 1, 2020

pavolloffay May 4, 2020

objectiser May 1, 2020

pavolloffay May 4, 2020

pavolloffay May 4, 2020

objectiser May 4, 2020

pavolloffay May 4, 2020

objectiser May 1, 2020

pavolloffay May 4, 2020

objectiser May 1, 2020

pavolloffay May 4, 2020

objectiser May 1, 2020

pavolloffay May 4, 2020

objectiser May 4, 2020

objectiser May 1, 2020

pavolloffay May 4, 2020

rubenvp8510 May 1, 2020

pavolloffay May 4, 2020 •

edited

Loading

objectiser left a comment

Merge hardcoded/default configuration with OTEL config file #2211

Merge hardcoded/default configuration with OTEL config file #2211

Conversation

pavolloffay commented Apr 29, 2020 • edited Loading

pavolloffay commented Apr 29, 2020

pavolloffay commented Apr 30, 2020

pavolloffay commented Apr 30, 2020

codecov bot commented Apr 30, 2020 • edited Loading

Codecov Report

pavolloffay commented May 1, 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

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

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 May 4, 2020 • edited Loading

Choose a reason for hiding this comment

objectiser left a comment

Choose a reason for hiding this comment

pavolloffay commented Apr 29, 2020 •

edited

Loading

codecov bot commented Apr 30, 2020 •

edited

Loading

pavolloffay May 4, 2020 •

edited

Loading