Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Getters and Setters being "optional helpers" passed to a TextMapPropagator is unclear #986

Open
arminru opened this issue Sep 22, 2020 · 4 comments
Assignees
Labels
area:api Cross language API specification issue question Question for discussion release:after-ga Not required before GA release, and not going to work on before GA spec:context Related to the specification/context directory

Comments

@arminru
Copy link
Member

arminru commented Sep 22, 2020

While reviewing #976, I noticed that the current spec for the TextMapPropagator specifies the Getters and Setters as:

Getter and Setter are optional helper components used for extraction and injection respectively (...)

Extract - Optional arguments:

  • A Getter invoked for each propagation key to get. This is an additional
    argument
    that languages are free to define to help extract data from the carrier.

Inject - Optional arguments:

  • A Setter invoked for each propagation key to add or remove. This is an additional
    argument
    that languages are free to define to help inject data into the carrier.

(bold added by me)

I don't quite understand how these can be "optional helpers" to "help" with things. I would rather deem them essential components used to extract from and inject into carriers. How could this be done without any Setter or Getter for interacting with the carrier?

cc @carlosalberto

@arminru arminru added question Question for discussion area:api Cross language API specification issue spec:context Related to the specification/context directory labels Sep 22, 2020
@carlosalberto
Copy link
Contributor

So overall it means that Getter and Setter are classes/interfaces that may be defined if the language maintainers deem them important (for the language at hand). Otherwise they could instead require that the user provides an additional interface/object such as:

class TextMapPropagator:
   Extract(Context context, ITextPayload payload):
      // ITextPayload is an interface wrapping a specific payload
      // and offering operations to fetch data from it.
      String value = payload.Get(key)

This was brought by @yurishkuro, hence cc-ing him here ;)

@arminru arminru added the release:after-ga Not required before GA release, and not going to work on before GA label Sep 22, 2020
@yurishkuro
Copy link
Member

There are two ways to define the propagation interfaces:

  1. The way OpenTracing did: Inject(ctx, Carrier), where the carrier implements a specific interface that provides access to the underlying metadata. It's the simplest API for propagator, but it requires that instrumentation almost always wraps the actual request objects into an adapter that exposes the carrier interface, which means extra memory allocation.

  2. The way Zipkin's Brave does it, which is similar to the idea of Getter/Setter: Inject(ctx, Setter<T>, T request). Here the setter is effectively a lambda that can be statically instantiated once, and the request object of whichever type is passed in directly.

Option 2 is usually more efficient, but it's not always needed, e.g. in some languages the request headers may already be represented by a native dictionary, thus no indirection or adapter is needed and Option 1 works fine.

@arminru
Copy link
Member Author

arminru commented Sep 23, 2020

Thanks for the clarification, @yurishkuro. So the spec is worded this way to allow SIGs to decide on either of both options, which seems reasonable to me - no need to restrict implementations here.

The wording with "helping" to set values is not some common terminology that I would expect in a technical specification but improving that is just an editorial change which we can address after GA as well.

@yurishkuro
Copy link
Member

@arminru I agree that this part of the spec as written is pretty cryptic and probably difficult to decipher by someone without a full background. Should not be this way; an explanation like above is sufficiently concise yet explains the rationale for dual API.

Agree though that it's not a blocker for GA, nice to have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue question Question for discussion release:after-ga Not required before GA release, and not going to work on before GA spec:context Related to the specification/context directory
Projects
None yet
Development

No branches or pull requests

3 participants