Add GZip encoding support to Zipkin

[ghost]

Is your feature request related to a problem? Please describe.
There should be a configurable option to send GZip-encoded spans to Zipkin endpoints.

Describe the solution you'd like

• add config property (on by default) to use GZip
• add code handling GZip of encoded byte array (of a span)

Describe alternatives you've considered

• none

Additional context

• none

[iNikem]

Before setting that ON by default, make sure that Collector can accept gzipped data

[anuraaga]

We use the okhttp sender which defaults to sending with gzip by default already. So is this about adding a config option? I think that makes sense, and I'd recommend adding an option for json vs proto too, we currently only have programmatic setting of that.

But how about sending a spec PR for the options first?

[iNikem]

I was not aware that we already use gzip :) Good to be us :) Now thinking about it, why do we need a config option for that? What is the use case to switch that off?

[ghost]

@anuraaga thanks for the tip - I took a brief look on the OkHttpSender but didn't spot the compressionEnabled option. So the situation right now is - if a customer uses OkHttpSender implementation, they will have GZIP enabled by default. I'll check if there is a use case for config option only and get back to you.

[anuraaga]

Thanks - times change and I think many environments have enough network bandwidth where gzip cost outweighs the benefit, so an option seems ok to me. But this is a good opportunity to also have an option for proto - zipkin had always defaulted to json for debugability but high ingestion sites benefit greatly from proto.

[iNikem]

WDYM by proto here? OTLP? Or Zipkin over protobuf?

[anuraaga]

Ah sorry - meant zipkin over protobuf. Zipkin supports both JSON and proto and easy to switch for the senders.

[iNikem]

The nearest thing in specification right now to exporters configuration is https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/sdk-environment-variables.md. I am not even sure where in spec should go the proposal to choose zipkin wire format: json/json+zip/protobuf.

[jkwatson]

Since the current exporter builder supports configuring both the Sender and the Encoder, can't the user set things up however they like, if they don't want the defaults?

[iNikem]

not autoinstrumentation's user :(

[jkwatson]

not autoinstrumentation's user :(

Hm. There's definitely a tension between providing every possible configuration option (via env var or sys prop, or whatever), and just providing the programmatic means for the agent (or other end user) to provide those options.

[iNikem]

Indeed. So you would prefer to have this configuration option in instrumentation repo?

[jkwatson]

Indeed. So you would prefer to have this configuration option in instrumentation repo?

I'm honestly not sure. What is the need of the original poster, @kubawach ? Is it needed for the general SDK, or is the agent enough for now? If it's only needed in the agent, I'd prefer to have it start there, and move it to the SDK if we have a need to configure the SDK that way.

[iNikem]

I think our original request was satisfied by the "discovery" that we already export gzipped zipkin :) So I think it is fair to close this ticket with resolution "either implement that switch in instrumentation repo or get it accepted to spec" :D

[ghost]

@jkwatson the idea was to have some flexibility regarding usage of GZip vs plain. As @anuraaga pointed out, sometimes it's not desirable to enable compression due to performance considerations. Other possible use case is to support customers using Zipkin collectors without compression support.
However as @iNikem already stated, for our current needs default GZip is good enough :)
Last but not least - IMO best way to handle this would be to have a property (env / sys) that would be processed by io.opentelemetry.exporters.zipkin.ZipkinSpanExporter.Builder same way as the current ones. Then the builder would configure the Sender accordingly. Interesting thing - the spec (https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/sdk-environment-variables.md) mentions following props for Zipkin:

• OTEL_EXPORTER_ZIPKIN_ENDPOINT
  while Builder::fromConfigMap processes following:
• otel.zipkin.service.name
• otel.zipkin.endpoint
  Not only there is another property but also the endpoint one has slightly different name (notice 'EXPORTER' token). Is this a real issue or I'm missing something (difference between the spec and actual env / sys props used to configure zipkin) ?

[jkwatson]

Given the flexibility of the exporter, it seems like a single property probably wouldn't cut it. You'd need at least several options for the various bits and pieces that can be customized for both the encoder and the sender.

And, yes, our exporter env vars are not up to spec for all the exporters.. They should be fixed to match. I'll create an issue for that.

[ghost]

@jkwatson perfect, so closing this little one.