Small clean up for Propagators.

[carlosalberto]

Updates #453

Things that still need to be addressed:

• Do we really need to separate Carrier and Setter (and maybe Getter)?: I had started working on it but it's no so trivial and will most likely need to become part of Propagation - Getter handling of multi-value headers/attributes #433 . I recommend working on it as part of that item.
• The Fields section needs to be clearer and not part of the HttpTextFormat: At this moment it is tied to HttpTextFormat, and might become more general depending on how Update Binary format in the Specification #437 is solved (not sure this can be used by Binary format, but still). I recommend creating a new ticket to track this, and depend on the outcome of Binary being restored.
• Add the notion of "Application Message" that semantically consists of metadata (e.g. HTTP headers) and the payload: I couldn't find the rationale for this, nor where it would be applied, and sadly cannot even remember about this anymore ;( Maybe @yurishkuro remembers?

[yurishkuro]

Sorry I haven't followed this part of the spec previously, but the Format/Carrier separation in OpenTracing was often criticized as unnecessary and confusing, leading to type-unsafe interfaces, and I was under impression that we were going to end up with a single entity in OTel. Specifically, the only role that the OpenTracing.Format played was informing the tracer of the runtime type of the Carrier. However, in OTel the Propagators are already typed & named after specific format, like HTTPPropagator, which allows them to accept strongly typed versions of the carriers. Therefore, I don't see what role the Format plays.

[carlosalberto]

Hey @yurishkuro thanks for the feedback.

the Format/Carrier separation in OpenTracing was often criticized as unnecessary and confusing"

So at this moment, the Format defines how Propagators subscribing to it should behave, i.e.

// HttpTextFormat
public void inject(Context context, Carrier carrier, Setter);

// Binary, although was temporarily removed
public void inject(Context context, byte[] buffer);

Which means there's no Format being passed, as it was in OpenTracing, as Format is in itself the definition of the contract/operation.

That being said, I feel I'm missing something from your comment. Would you mind elaborating?

[yurishkuro]

public void inject(Context context, Carrier carrier, Setter);

So there's even one more thing here, a Setter. Wouldn't this be simpler?

public interface HTTPPropagator {
    public void inject(Context context, HTTPCarrier carrier);
}

While the "format" as a concept is sort of present here, but also sort-of not, because any term/concept we introduce in the spec creates an unnecessary mental overhead. In OT the Format was actually strongly typed in some languages, but I think the whole model would be simpler if we talk about Propagators/Carriers as abstract notions with concrete subtypes suitable for different transports.

[carlosalberto]

So there's even one more thing here, a Setter. Wouldn't this be simpler?

I think it will, and it will probably happen as part of #433 anyway (at least we can revisit that possibility).

I think the whole model would be simpler if we talk about Propagators/Carriers as abstract notions with concrete subtypes suitable for different transports.

Ok, so kinda drop the Format notion. I will spend some time later today or tomorrow re-working this, and see how it goes from there (just to be clear, I really want to keep a clear mention of HttpTextFormat and Binary).

[carlosalberto]

Hey @yurishkuro

I think the whole model would be simpler if we talk about Propagators/Carriers as abstract notions with concrete subtypes suitable for different transports.

So I updated this PR to make a Format an abstract contract used for a given transport, with general operations (Inject/Extract), and have Propagators implement specific Formats (along with their specific carrier types).

With these changes, HttpTextFormat is a contract that uses text as its carrier type, and that's it.

Let me know if you think we should remove the few remaining Format bits (in that case, we could try to drop entirely the Format term, and use TextMap Propagator instead of Format all over the place, and see how it feels).

(Also, the refactoring should help a lot when we restore the Binary format).

[yurishkuro]

I think "carrier" is an abstraction of inter-process message that also directly implies the metadata format via the API that specific carriers expose. So on one hand we could try to express everything in terms of carriers, but on the other hand I did just use the words "metadata format". Maybe it's ok to keep those, but I would suggest trying to avoid using fixed-width Format and referring to it as some kind of object in the API, i.e. just keep it as an introduction section, not as an entity introduction.

[carlosalberto]

Hey @yurishkuro thanks for the answer.

So I'm trying to get the exact details done.

I would suggest trying to avoid using fixed-width Format and referring to it as some kind of object in the API, i.e. just keep it as an introduction section, not as an entity introduction.

Does this mean we should just barely mention Format, maybe as an interface rather than a 'contract' (so it feels more like an actual API object), and not list any operations/details? *If this is the case, we can do that, although I find useful to define the 'base' behavior that all Formats expect (for Inject/Extract, that is, as they will also be present in the Binary format).

If not, please elaborate a little bit more ;) I'm happy to make the iterations needed to get us to a much better place, but would like to get a better understanding of your vision.

[yurishkuro]

Does this mean we should just barely mention Format, maybe as an interface rather than a 'contract'

I would only mention it when discussing "metadata format". I don't see why it needs to be any symbol in the API - why would it be an interface? The Inject/Extract methods are on the Propagator, not the format.

[carlosalberto]

Hey @yurishkuro Updated the propagators section to not use Format, but simple say mention Propagator types, e.g. HttpTextPropagator.

Not sure this is how you had envisioned this one. But we definitely want to keep mentioning the details of the HttpText propagator type, so we can't leave it entirely abstract.

Let me know ;)

[carlosalberto]

@yurishkuro Updated. The only remaining item is the issue regarding Propagators taking a Context as an optional argument. Please review and let's iterate on that along with @Oberon00 - that being said, personally I'd be fine with removing that line (and adding it later if/as needed ;) )

[Oberon00]

A changelog.md change is also required.

[carlosalberto]

@Oberon00 Updated. Please leave feedback on the new changes.

[Oberon00]

Looks like a good improvement to me overall. I hope this unblocks #437 😃

[Oberon00]

Maybe it would be more clear to define fields as the key-value pairs and then use field key or field name below.

[carlosalberto]

I'd rather so that as a follow up in order to make progress and merge this PR ;)

Trying to clarify: you are suggesting we only change the terms used (and update the document with them), not the actual return value (a list of strings), right?

[Oberon00]

Yes, I was just trying to clarify terminology, not suggesting an actual content change.

[carlosalberto]

@yurishkuro Removed the (hopefully final) requested change, regarding the default Context for Injection/Extraction (it won't hurt not having this clarification, and if truly needed in the future we can always bring it back). Please review and let me know ;)

[bogdandrutu]

@carlosalberto please rebase

[carlosalberto]

@bogdandrutu @yurishkuro Ready for review ;)

[yurishkuro]

#711

[yurishkuro]

I find this Fields section very confusing and not well-worded (e.g. what does "Returns..." refer to?) Let's revisit it in another PR. #712

[yurishkuro]

#433 is about multi-valued headers, different from iterator. I booked #713.