Disable Zipkin server if port/address is not configured

[yurishkuro]

In the past Zipkin server would not be started if the port was not configured, but not it's always started.

• run OTEL tests as part of test-ci (without coverage)
• change CollectorZipkinHTTPHostPort default to ""
• add logging when Zipkin server is not started
• changed ports.GetAddressFromCLIOptions() to return empty string when port=0 && hostPort="" (previously it was returning ":", which didn't make sense, it's better to return default value for string when no parts of the address are specified)

With this change:

$ go run ./cmd/collector help | grep -i zipkin
      --collector.zipkin.host-port string               The host:port (e.g. 127.0.0.1:9411 or :9411) of the collector's Zipkin server (disabled by default)

$ SPAN_STORAGE_TYPE=memory go run ./cmd/collector 2>&1 | grep -i zipkin
{"level":"info","ts":1602520683.32179,"caller":"server/zipkin.go:45","msg":"Not listening for Zipkin HTTP traffic, port not configured"}

$ SPAN_STORAGE_TYPE=memory go run ./cmd/collector --collector.zipkin.host-port 9411 2>&1 | grep -i zipkin
{"level":"info","ts":1602520719.572457,"caller":"server/zipkin.go:49","msg":"Listening for Zipkin HTTP traffic","zipkin host-port":":9411"}

[joe-elliott]

This will break at least two things in the otel collector/agent. I can review after this is merged and provide a PR if that's easiest.

This test will need to be corrected to the new value:
https://github.com/jaegertracing/jaeger/blob/master/cmd/opentelemetry/app/receiver/zipkinreceiver/zipkin_receiver_test.go#L42

This check will need to updated to the new default:
https://github.com/jaegertracing/jaeger/blob/master/cmd/opentelemetry/app/defaultconfig/default_config.go#L167

[codecov]

Codecov Report

Merging #2554 into master will decrease coverage by 0.02%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master    #2554      +/-   ##
==========================================
- Coverage   95.36%   95.34%   -0.03%     
==========================================
  Files         208      208              
  Lines        9251     9254       +3     
==========================================
+ Hits         8822     8823       +1     
- Misses        353      354       +1     
- Partials       76       77       +1     

Impacted Files	Coverage Δ
cmd/collector/app/server/zipkin.go	0.00% <0.00%> (ø)
cmd/collector/app/builder_flags.go	100.00% <100.00%> (ø)
ports/ports.go	100.00% <100.00%> (ø)
cmd/query/app/server.go	88.52% <0.00%> (-1.64%)	⬇️
plugin/storage/integration/integration.go	77.34% <0.00%> (-0.56%)	⬇️
...lugin/sampling/strategystore/adaptive/processor.go	100.00% <0.00%> (+0.92%)	⬆️

---

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 b754fbb...aa4ffc4. Read the comment docs.

[yurishkuro]

Hold on, how is it that you're saying tests will need to be fixed but they are not failing in the CI? Are we not running them at all in travis?

[yurishkuro]

Added a fix to invoke OTEL tests from test-ci (larger issue in #2555)

[yurishkuro]

Confirmed that OTEL tests are now running and failing with this change (https://travis-ci.org/github/jaegertracing/jaeger/jobs/735111015), so fixed it by changing how GetAddressFromCLIOptions works.

[joe-elliott]

nit: This line

https://github.com/jaegertracing/jaeger/blob/master/cmd/opentelemetry/app/defaultconfig/default_config.go#L167

could be simplified to:

if zipkin.Endpoint != "" {

Other than that, looks good to me.