Invalid expectations of JSON format for Jaeger remote sampling strategies

[yurishkuro]

What are you trying to achieve?

Make OpenTelemetry SDKs work with Jaeger remote sampling (raised in discussion jaegertracing/jaeger#4142).

Problem

The OpenTelemetry specification did not fully specify the requirements for samplers that support Jaeger remote sampling protocol. Specifically, it referred to gRPC sampling.proto making it look like this is the format that samplers must support. However, this is not the protocol that was used by the original Jaeger SDK samplers.

A bit of history:

• At inception the Jaeger backend used Thrift v0.9.2 as the main RPC format. Since samplers ran in client space and did not always have access to Thrift libs, they used JSON format for the payload, which was produced by the backend by serializing Thrift objects as JSON. The strategies were served from jaeger_agent::5778/ endpoint, and the output could like like this: {"strategyType":0, "probabilisticSampling":{"samplingRate":1}}. Note that the strategyType enum is represented as a number. Also note that the agents only served the endpoint, but the actual strategy for a service was determined by the Jaeger collector, and agents simply proxied the SDKs HTTP requests to collector's Thrift endpoint.
• Later the Jaeger backend was upgraded to Thrift v0.9.3, which made a breaking change in JSON serialization by returning enum names instead of enum numbers. The new format would look like this: {"strategyType":"PROBABILISTIC", "probabilisticSampling":{"samplingRate":1}}. The Jaeger SDKs were upgraded to parse this format, but to maintain backwards compatibility with existing SDKs in production the new format was instead exposed at a new endpoint jaeger_agent::5778/sampling. The / endpoint still continued to return the old format.
• Later the Jaeger backend moved to gRPC as the main RPC format, including all communications between the agents and the collectors. This is when gRPC sampling.proto was introduced, but it was not used by any of Jaeger SDKs, and the JSON format that could be inferred from this proto definition was never used by any Jaeger components.

The current state in OpenTelemetry:

• The jaeger-remote-sampler in the Java SDK uses binary gRPC format. This is different from what Jaeger SDKs used to do, but no particular issues here since gRPC sampling.proto should be considered official (even though it's not documented as such https://www.jaegertracing.io/docs/1.41/apis/, to be fixed fixed). Curiously, it is also using a custom-written parser for proto, instead of the standard libs.
• The jaegerremote sampler in the Go SDK uses a standard encoding/json module to parse the JSON payload into a struct generated from sampling.proto, which means it expects enums as integers. That format has never been used by the Jaeger backend, but coincidentally it matches the deprecated Thrift v0.9.2 format. Thus, the Go SDK seems to work against the deprecated jaeger_agent::5778/ endpoint, but not OTel Collector. [samplers/jaegerremote] Invalid expectation of JSON format opentelemetry-go-contrib#3184
• The Rust SDK seems to be similar to Go SDK, retrieving payload from HTTP.
  • Some questions about trace/sampler/jaeger_remote opentelemetry-rust#954
• I did not find support for Jaeger remote sampling in the other language SDKs.
• OpenTelemetry Collector has a contrib module jaegerremotesampling that serves Thrift-generated struct (not Protobuf-generated, like Go SDK) serialized with standard encoding/json module, both on the legacy / endpoint and on the "official" Jaeger endpoint /sampling
  • [extension/jaegerremotesampling] Misleading implementation on two endpoints opentelemetry-collector-contrib#18058
  • [extension/jaegerremotesampling] This extension may be broken after importing Jaeger v1.42 code opentelemetry-collector-contrib#18047

How to proceed from here

• Change Jaeger to use standard Protobuf-to-JSON serialization on /sampling (agent) and /api/sampling (collector) endpoints, which is backwards compatible with Thrift-based serialization
• Update documentation in OpenTelemetry SDKs to use these endpoints instead of the legacy jaeger_agent::5778/
• Add unit tests to the SDKs that actually try to parse the strategies from a string
• Fix collector's inconsistency [extension/jaegerremotesampling] Misleading implementation on two endpoints opentelemetry-collector-contrib#18058
• Fix Go SDK [samplers/jaegerremote] Invalid expectation of JSON format opentelemetry-go-contrib#3184

[yurishkuro]

cc @TommyCpp (Rust SDK)
cc @anuraaga (Java SDK)
cc @kvrhdn (Go SDK)

[yurishkuro]

cc @jpkrohling (Collector)

[TommyCpp]

Rust implementation uses the HTTP response as JSON based on proto, it expects the enum to be string. So the example response {"strategyType":"PROBABILISTIC", "probabilisticSampling":{"samplingRate":1}} should work for Rust jaeger remote sampler. So the proposed change should work for us.

We can support encoding enum as integer too.

[jpkrohling]

cc @frzifus, as the code owner for Jaeger components in the collector