Mark legacy exporter features in spec compliance optional. SDKs are free to su…

[anuraaga]

…pport it if they'd like.

Fixes #1356

Changes

Removes jaeger-thrift-http from compliance matrix

[anuraaga]

I left jaeger thrift UDP since my reading of the jaeger docs made it seem it's still a good idea for jaeger in agent mode, but I don't know if that's actually true. Let me know if it makes sense.

[Oberon00]

I left jaeger thrift UDP since my reading of the jaeger docs made it seem it's still a good idea for jaeger in agent mode, but I don't know if that's actually true. Let me know if it makes sense.

Even if it is a good idea, it still doesn't sound like it would be essential.

[iNikem]

If compliance matrix represents what SDKs MUST implement for GA, then the question is "why do we have Jaeger Thrift UDP in there?" Is it to satisfy some backward compatibility? Or there is an actual business use case for it?

If there are no good answers to that question, then we should remove it to simplify GA.

[yurishkuro]

I think this change takes away from the project by hiding the features that some SDK maintainers already spent effort on implementing. Instead, it would make more sense to me to declare certain features as optional for SDKs to implement, but still have the features matrix where one can compare SDKs across many dimensions.

Cf. https://www.jaegertracing.io/docs/1.21/client-features/

[iNikem]

@yurishkuro That is a very good idea. But I heard several times that "All language SIGs MUST implement everything in spec compliance matrix before GA". Is TC willing to change this stance and to bring optional components into the matrix?

[Oberon00]

The spec compliance matrix is geared towards maintainers and release mangagers, not external users IMHO. But I'm OK with moving this to some "optional features" section.

[yurishkuro]

There are plenty of - and empty cells in this matrix, I feel like "MUST implement" is overstating it. Makes even more sense to designate some rows as required/optional.

The spec compliance matrix is geared towards maintainers and release managers, not external users

At some point there will need to be a public (and pretty) version of the features matrix similar to that Jaeger page. I think this matrix should be treated as a source of truth.

[Oberon00]

There are plenty of - and empty cells in this matrix, I feel like "MUST implement" is overstating it.

I think most of the rows do correspond to (one or more) MUST-requirements. But it can't hurt to check.

[carlosalberto]

+1 to moving it to an optional section (at the bottom probably?)

[yurishkuro]

to moving it to an optional section (at the bottom probably?)

I would rather add a Require/Optional column, it is easier to read the table when semantically relevant entries are grouped together.

[anuraaga]

Ok - added an optional column instead and my interpretation of what seems optional, please make suggestions as needed.

[anuraaga]

The optional ones are meant to indicate what's optional long term, this isn't meant to tie to 1.0 or GA requirements in any way.

[anuraaga]

I'm not really sure what this spec is :) As no SDK has implemented it yet assuming it's optional.

[Oberon00]

I would rather add a Require/Optional column, it is easier to read the table when semantically relevant entries are grouped together.

I am against that. The table is large enough as it is. This column would invite all minor optional features to be added, even if they are just in one SDK.

[yurishkuro]

This column would invite all minor optional features to be added, even if they are just in one SDK.

I doubt it. Features in the matrix must be spelled out elsewhere in the spec, and we can always turn down such changes if they are not general enough.

[Oberon00]

Features in the matrix must be spelled out elsewhere in the spec,

That's a good point, I think that should be the case. But I cannot find "thrift" in https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk_exporters/jaeger.md

[jkwatson]

is this mandatory?

[anuraaga]

I figure if an exporter is called twice concurrently there's an expectation that the two sends would happen at the same time in any language.

[jkwatson]

is this required? If an SDK allows people to configure the grpc channel however they want, does that fulfill this?

[anuraaga]

Handling retry well seems like a nice feature to support eventually, though not necessarily for 1.0 (which was my intent). I'd think being able to configure the gRPC channel could be enough, and shortcuts could be added based on user demand independently of this spec.

[mwear]

I don't oppose this in principal, but I do oppose several of the choices as to which features are required and which are optional, especially at this stage of the game. Ruby chose proto over http for its OTLP exporter and Thrift for the Jaeger exporter. We do not have gRPC versions of either, which are listed as required. Ideally, we'd have a less restrictive policy, such as, you need an OTLP and Jaeger exporter, but you choose whichever transport makes most sense for your SIG. cc: @fbogsany.

[iNikem]

@mwear What do you mean by "choices"? This PR marks some features which were required for GA as now optional, relaxing the requirements. It does not bring any new requirements on the table.

It was said several times during both maintainers and spec meetings that ALL rows in spec compliance matrix are required for GA. If some SIGs find some of those requirements unreasonable, please raise that concern loud and clear. Probably as separate issue/PR.

[anuraaga]

It was said several times during both maintainers and spec meetings that ALL rows in spec compliance matrix are required for GA.

I actually didn't know this either :) In that case, I feel like I need to mark most of the retry stuff as optional, and add some text that for each type of exporter, only one format is required for GA.

[iNikem]

It was said several times during both maintainers and spec meetings that ALL rows in spec compliance matrix are required for GA.

I actually didn't know this either :) In that case, I feel like I need to mark most of the retry stuff as optional, and add some text that for each type of exporter, only one format is required for GA.

I am happy to be wrong here :) @open-telemetry/technical-committee can you confirm or deny that "ALL rows in spec compliance matrix are required for GA"?

[carlosalberto]

In that case, I feel like I need to mark most of the retry stuff as optional, and add some text that for each type of exporter, only one format is required for GA.

That's correct - I think, as time goes by, we may make more items mandatory, i.e. more exporter protocols. But for now we need to make more items optional.

[carlosalberto]

I am happy to be wrong here :) @open-telemetry/technical-committee can you confirm or deny that "ALL rows in spec compliance matrix are required for GA"?

That was the plan IIRC, yes. In that regard, the Matrix can help us know whether we are complete and ready to release or not ;)

[carlosalberto]

Ideally, we'd have a less restrictive policy, such as, you need an OTLP and Jaeger exporter, but you choose whichever transport makes most sense for your SIG.

During today's Spec call we decided to go with this decision. Hope that makes sense @anuraaga ;)

[bogdandrutu]

Should we use YES instead of X for the optional values?

[anuraaga]

Have no preference for either, so for now sticking with what's here unless someone has stronger preference :)

[arminru]

I'd prefer just writing Optional in there (the column has the width of the header anyway), so one would not have to look up the heading. Anyway, I'd rather merge this asap to avoid conflicts and consider prettifying it afterwards in a smaller changeset.

[SergeyKanzhelev]

@jkwatson @anuraaga any objections for the current version? There are couple outdated unresolved comments, thus the question. If no input for a day, I'd consider there is no objections.

[carlosalberto]

@SergeyKanzhelev We still need to make this clarification:

you need an OTLP and Jaeger exporter, but you choose whichever transport makes most sense for your SIG.

[anuraaga]

Made some updates

• Added an asterisk note that one of the supported formats of an exporter type is required
• Didn't include Zipkin V1 in this, I think V1 is not enough to be conidered supporting Zipkin and that is truly optional, one of the V2 formats is required
• Took the liberty to mark the retry / throttling stuff optional, I'd be surprised if we want to block SIGs on going GA for them, in some languages it seems like it could be pretty tough.

[arminru]

I'd prefer just writing Optional in there (the column has the width of the header anyway), so one would not have to look up the heading. Anyway, I'd rather merge this asap to avoid conflicts and consider prettifying it afterwards in a smaller changeset.

[arminru]

C++: + to - on purpose?

[carlosalberto]

Ooops, I just realized we just had a race condition, my bad :( Would you mind filling issues (or PRs) to follow these two details up?

[arminru]

No worries, I think it's fine actually, as it seems to be indeed missing:
https://github.com/open-telemetry/opentelemetry-cpp/blob/main/api/include/opentelemetry/trace/span.h
The other change is just cosmetic, nothing urgent.

[carlosalberto]

Merging this now as we have enough approvals. @bogdandrutu please let me know if you still prefer a YES instead of an X for the Optional column, as I can easily cook a PR myself.