Fix TLS flags settings in jaeger OTEL receiver

[pavolloffay]

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

The GRPC TLS settings were not applied when --collector.grpc.host-port= was not provided. This was causing issues in the operator as it does not set the endpoint.

[codecov]

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2438      +/-   ##
==========================================
- Coverage   95.57%   95.55%   -0.02%     
==========================================
  Files         208      208              
  Lines       10682    10682              
==========================================
- Hits        10209    10207       -2     
- Misses        400      402       +2     
  Partials       73       73              

Impacted Files	Coverage Δ
cmd/query/app/static_handler.go	81.66% <0.00%> (-2.50%)	⬇️
plugin/storage/integration/integration.go	80.38% <0.00%> (-0.48%)	⬇️
cmd/query/app/server.go	93.33% <0.00%> (+2.22%)	⬆️

---

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 af985ae...1ce4466. Read the comment docs.

[joe-elliott]

Final question. Since GRPC and Thrift HTTP are automatically enabled on the default jaeger collector should we always start the otel collector receivers?

[joe-elliott]

can simplify if cOpts.TLS.Enabled {

Should enabling TLS through the Jaeger Collector options also turn on TLS for HTTP below? Ignore. I see that the collector settings are specific to GRPC.

[pavolloffay]

Final question. Since GRPC and Thrift HTTP are automatically enabled on the default jaeger collector should we always start the otel collector receivers?

They are started/configured by the defaultconfig package. I would not enable them here by default to keep the config in sync with the OTEL upstream. E.g. we might want to provide a distribution with the same default config as OTEL upstream #2281 (comment) (e.g the root command would behave like otelcol)

[joe-elliott]

Ok, It took some unwinding, but I see what you're referring to. It looks like the order is:

1. Start with OTEL defaults for the receiver
   

   jaeger/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver.go

   Line 55 in 5fb5e8a

   cfg := f.Wrapped.CreateDefaultConfig().(*jaegerreceiver.Config)

2. Apply Viper (Jaeger) Config
   

   jaeger/cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver.go

   Line 62 in 5fb5e8a

   configureCollector(f.Viper, cfg)

3. Fill out Jaeger Defaults
   

   jaeger/cmd/opentelemetry/app/defaultconfig/default_config.go

   Line 127 in 5fb5e8a

   if jaeger.GRPC == nil {

I'm wondering if steps two and three should be swapped. For instance in this PR if cfg.GRPC == nil we are subbing in the default Jaeger GRPC settings which we then do again in step 3.

we might want to provide a distribution with the same default config as OTEL upstream

Is there a default config for OTEL? Last time I checked the operator had to configure the basic pipeline.

Obviously there is a lot of history here and I'm just playing catch up. I will continue to think about this as I review the code/PRs!

[pavolloffay]

I'm wondering if steps two and three should be swapped. For instance in this PR if cfg.GRPC == nil we are subbing in the default Jaeger GRPC settings which we then do again in step 3.

Maybe it could be swapped or even merged together. Maybe at the default conf creation time we know what componnet (collector, agent...) is being started.

Is there a default config for OTEL? Last time I checked the operator had to configure the basic pipeline.

There isn't there is also a proposal to have one, but It probably won't be merged. Maybe OTEL will provide a default config file with the distribution.