Add zipkin receiver to OTEL collector #2181

pavolloffay · 2020-04-15T14:31:09Z

Add Zipkin receiver to OTEL collector.

The Zipkin receiver is disabled by default and it can be enabled by flags:

      --collector.zipkin.host-port string          The host:port (e.g. 127.0.0.1:5555 or :5555) of the collector's Zipkin server (default ":0")
      --collector.zipkin.http-port int             (deprecated, will be removed after 2020-06-30 or in release v1.20.0, whichever is later) see --collector.zipkin.host-port

Other notable changes:

All collector flags are added to OTEL collector. Most of them are be noop exept zipkin. We could support other flags (grpc endpoint, HTTP endpoint) also but it will require changes in OTEL jaeger receiver.

      --collector.grpc-port int                    (deprecated, will be removed after 2020-06-30 or in release v1.20.0, whichever is later) see --collector.grpc-server.host-port
      --collector.grpc-server.host-port string     The host:port (e.g. 127.0.0.1:5555 or :5555) of the collector's GRPC server (default ":14250")
      --collector.grpc.tls                         (deprecated) see --collector.grpc.tls.enabled
      --collector.grpc.tls.cert string             Path to a TLS Certificate file, used to identify this server to clients
      --collector.grpc.tls.client-ca string        Path to a TLS CA (Certification Authority) file used to verify certificates presented by clients (if unset, all clients are permitted)
      --collector.grpc.tls.client.ca string        (deprecated) see --collector.grpc.tls.client-ca
      --collector.grpc.tls.enabled                 Enable TLS on the server
      --collector.grpc.tls.key string              Path to a TLS Private Key file, used to identify this server to clients
      --collector.http-port int                    (deprecated, will be removed after 2020-06-30 or in release v1.20.0, whichever is later) see --collector.http-server.host-port
      --collector.http-server.host-port string     The host:port (e.g. 127.0.0.1:5555 or :5555) of the collector's HTTP server (default ":14268")
      --collector.num-workers int                  The number of workers pulling items from the queue (default 50)
      --collector.queue-size int                   The queue size of the collector (default 2000)
      --collector.queue-size-memory uint           (experimental) The max memory size in MiB to use for the dynamic queue.
      --collector.tags string                      One or more tags to be added to the Process tags of all spans passing through this collector. Ex: key1=value1,key2=${envVar:defaultValue}
      --collector.zipkin.allowed-headers string    Comma separated list of allowed headers for the Zipkin collector service, default content-type (default "content-type")
      --collector.zipkin.allowed-origins string    Comma separated list of allowed origins for the Zipkin collector service, default accepts all (default "*")
      --collector.zipkin.host-port string          The host:port (e.g. 127.0.0.1:5555 or :5555) of the collector's Zipkin server (default ":0")
      --collector.zipkin.http-port int             (deprecated, will be removed after 2020-06-30 or in release v1.20.0, whichever is later) see --collector.zipkin.host-port

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

objectiser · 2020-04-15T15:43:08Z

@pavolloffay Wanted to clarify your comment:

We could support other flags (grpc endpoint, HTTP endpoint) also but it will require changes in OTEL jaeger receiver.

Wouldn't those flags just map their values onto the Jaeger receiver in the OTC config? e.g. https://github.com/open-telemetry/opentelemetry-collector/tree/master/receiver#communicating-over-tls - what changes would be required in OTEL jaeger receiver?

pavolloffay · 2020-04-15T16:17:42Z

Wouldn't those flags just map their values onto the Jaeger receiver in the OTC config? e.g.

Yes, that is my idea. I am not sure how it will be done. We need access to the receiver configuration object before it gets used by the factory. I will be looking into this shortly.

rubenvp8510 · 2020-04-15T17:32:26Z

The tests on https://github.com/jaegertracing/jaeger/blob/master/cmd/opentelemetry-collector/app/defaults/default_config_test.go#L96 should be updated.

But this looks good so far

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

pavolloffay · 2020-04-16T10:09:54Z

This is ready for re-review. For the flags I have created #2182

objectiser · 2020-04-16T10:30:55Z

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

+	recvs := map[string]configmodels.Receiver{
+		"jaeger": jaeger,
+	}
+	if zipkinHostPort != disabledZipkinHostPort {


Rather than define a special port, would it be better to just have a pointer? So test against nil.

We could do that too, although this uses the same approach as other parts of the codebase - zipkin is disabled if :0 is used. This value is also set as the default of the flag.

I did a small change, I have removed the const and used a built-in func to return the value of disabled endpoint.

objectiser · 2020-04-16T10:31:36Z

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

 // Config creates default configuration.
 // It enables default Jaeger receivers, processors and exporters.
-func Config(storageType string, factories config.Factories) (*configmodels.Config, error) {
+func Config(storageType string, zipkinHostPort string, factories config.Factories) (*configmodels.Config, error) {


Would it be better to use an interface so can just add extra flag fields if/when supported?

Interface perhaps no but options/params struct could be used. I would wait and see until the number of parameters grows and then refactor. Three params still look good to me.

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

codecov · 2020-04-16T13:07:06Z

Codecov Report

Merging #2181 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2181   +/-   ##
=======================================
  Coverage   96.14%   96.14%           
=======================================
  Files         219      219           
  Lines       10585    10585           
=======================================
  Hits        10177    10177           
  Misses        352      352           
  Partials       56       56

Impacted Files	Coverage Δ
cmd/query/app/server.go	`91.78% <0.00%> (-2.74%)`	⬇️
plugin/storage/badger/spanstore/reader.go	`96.79% <0.00%> (+0.64%)`	⬆️

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 72f5a6b...335f69c. Read the comment docs.

* Add zipkin receiver to OTEL collector Signed-off-by: Pavol Loffay <[email protected]> * fmt Signed-off-by: Pavol Loffay <[email protected]> * Fix tests Signed-off-by: Pavol Loffay <[email protected]> * Use host port instead of const Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay added 2 commits April 15, 2020 16:17

Add zipkin receiver to OTEL collector

0cf9662

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

fmt

989ef81

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

pavolloffay requested a review from a team as a code owner April 15, 2020 14:31

pavolloffay requested review from jpkrohling and objectiser April 15, 2020 14:31

Fix tests

073b0f9

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

pavolloffay mentioned this pull request Apr 16, 2020

OTEL collector exposes flags that are not used #2182

Closed

objectiser reviewed Apr 16, 2020

View reviewed changes

objectiser approved these changes Apr 16, 2020

View reviewed changes

Use host port instead of const

335f69c

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

objectiser approved these changes Apr 16, 2020

View reviewed changes

pavolloffay merged commit d75eb14 into jaegertracing:master Apr 16, 2020

pavolloffay mentioned this pull request May 14, 2020

Configure Jaeger receiver and exporter by flags #2241

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add zipkin receiver to OTEL collector #2181

Add zipkin receiver to OTEL collector #2181

pavolloffay commented Apr 15, 2020

objectiser commented Apr 15, 2020

pavolloffay commented Apr 15, 2020

rubenvp8510 commented Apr 15, 2020

pavolloffay commented Apr 16, 2020

objectiser Apr 16, 2020

pavolloffay Apr 16, 2020

pavolloffay Apr 16, 2020

objectiser Apr 16, 2020

pavolloffay Apr 16, 2020

codecov bot commented Apr 16, 2020 •

edited

Loading

Add zipkin receiver to OTEL collector #2181

Add zipkin receiver to OTEL collector #2181

Conversation

pavolloffay commented Apr 15, 2020

objectiser commented Apr 15, 2020

pavolloffay commented Apr 15, 2020

rubenvp8510 commented Apr 15, 2020

pavolloffay commented Apr 16, 2020

objectiser Apr 16, 2020

Choose a reason for hiding this comment

pavolloffay Apr 16, 2020

Choose a reason for hiding this comment

pavolloffay Apr 16, 2020

Choose a reason for hiding this comment

objectiser Apr 16, 2020

Choose a reason for hiding this comment

pavolloffay Apr 16, 2020

Choose a reason for hiding this comment

codecov bot commented Apr 16, 2020 • edited Loading

Codecov Report

codecov bot commented Apr 16, 2020 •

edited

Loading