[jaeger-v2] Align Query Service Config With OTEL

[mahadzaryab1]

Requirement

In this issue, we want to align the configuration for the query service with OTEL (part of #5229)

Tasks / outcomes

• All config fields are tagged with mapstructure such that they can be addressable from YAML via "good" names
• Configs implement Validate()
• Configs via YAML support the same defaults as in jaeger-v1
• Configs reuse standard elements of OTEL configs whenever possible, e.g. TLS, clientcfg, etc.
• Configuration looks "logical" and properly grouped, not flattened with long weird names
• We create a migration guide document with a table that lists v1 CLI flags and the corresponding v2 config fields

[yurishkuro]

We create a migration guide document with a table that lists v1 CLI flags and the corresponding v2 config fields

this box is checked, is it correct?

[mahadzaryab1]

We create a migration guide document with a table that lists v1 CLI flags and the corresponding v2 config fields

this box is checked, is it correct?

@yurishkuro The migration guide is linked in the PR description. Is that what you were asking? Let me know if I misunderstood.

[mahadzaryab1]

@yurishkuro Should we do the migration for v1 to OTEL HTTP/GRPC configurations as part of this issue?

[yurishkuro]

Sorry, not seeing migration guide.

On migrating servers - would be nice, minimize the differences. But ok to leave as is too.

[mahadzaryab1]

Sorry, not seeing migration guide.

@yurishkuro Here it is. It was linked in the PR. My apologies if it wasn't very visible.

[mahadzaryab1]

On migrating servers - would be nice, minimize the differences. But ok to leave as is too.

@yurishkuro It looks pretty doable for the most part. There's just one part that isn't super compatible. AdditionalHeaders in v1 is of type map[string][]string. On the other hand, ResponseHeaders is of type map[string]configopaque.String. Do you have any thoughts on this and if there's a way to consolidate them?

[yurishkuro]

We can converge to OTEL ones. I doubt if our cli actually allows to specify []string for each header.

[yurishkuro]

It was linked in the PR. My apologies if it wasn't very visible.

Thanks, I copied the migration guide to a document I own https://docs.google.com/document/d/1ydTrEtIqJZxGb0RBkUAW-ZIaHNijo1pZSrn1TDSK-LU/edit#

[mahadzaryab1]

It was linked in the PR. My apologies if it wasn't very visible.

Thanks, I copied the migration guide to a document I own https://docs.google.com/document/d/1ydTrEtIqJZxGb0RBkUAW-ZIaHNijo1pZSrn1TDSK-LU/edit#

No worries! I've done the same for the rest of the PRs in case you wanted to copy those over as well.

[mahadzaryab1]

It was linked in the PR. My apologies if it wasn't very visible.

Thanks, I copied the migration guide to a document I own https://docs.google.com/document/d/1ydTrEtIqJZxGb0RBkUAW-ZIaHNijo1pZSrn1TDSK-LU/edit#

@yurishkuro This is how the flag is specified

flagSet.Var(&config.StringSlice{}, queryAdditionalHeaders, `Additional HTTP response headers.  Can be specified multiple times.  Format: "Key: Value"`)

And initialized as follows:

stringSlice := v.GetStringSlice(queryAdditionalHeaders)
headers, err := stringSliceAsHeader(stringSlice)
if err != nil {
logger.Error("Failed to parse headers", zap.Strings("slice", stringSlice), zap.Error(err))
} else {
qOpts.AdditionalHeaders = headers
}

For mapping purposes, is it safe to only take the first element of the slice?

[yurishkuro]

Format: "Key: Value"

I added a log statement and tried this:

$ SPAN_STORAGE_TYPE=memory go run ./cmd/query --query.additional-headers 'k:v' --query.additional-headers 'k1:v1,v2'

logged:

"msg":"Using response headers","headers":{"K":["v"],"K1":["v1,v2"]}}

So CLI allows giving the flag multiple times, and as a result it does not use , to split entries, but uses it to split values for a key (effectively replicating HTTP parsing, even uses the parser internally).

The OTEL's implementation does not really check what's inside the string, so it probably also supports multiple values. The response headers handler is automatically mounted by confighttp.ServerConfig.ToServer() - are we actually using this in query service? I think it would be nice if we did, but that might be a much bigger change (although worth it going forward).

[mahadzaryab1]

@yurishkuro I see. So for mapping from CLI to v2, can we just take the first element of the slice and populate it in OTEL's response headers?

We currently aren't using ToServer in the query service. Is that something that we want to tackle in this issue?

[yurishkuro]

So for mapping from CLI to v2, can we just take the first element of the slice and populate it in OTEL's response headers?

No, you can join all values in a slice with a comma, that's how HTTP supports multi-valued headers

[yurishkuro]

We currently aren't using ToServer in the query service. Is that something that we want to tackle in this issue?

I think it's interesting to see how well this change would work. We may actually run into functional gap - we allow running http and grpc server on the same port, but OTEL does not. This will be a breaking change for v2 anyway, we might as well do it now.

[mahadzaryab1]

@yurishkuro anything else that we want to address as part of this issue? do we want to make the ToServer change here or track that separately?

[yurishkuro]

yes, we can track that separately, it's not a high priority. #6026

[mahadzaryab1]

perfect! can we close this issue out then?

[yurishkuro]

yes, thanks!