Separate TextMapCarrier into Injector and Extractor interfaces

[kyleholohan]

Before opening a feature request against this repo, consider whether the feature should/could be implemented in the other OpenTelemetry client libraries. If so, please open an issue on opentelemetry-specification first.

Is your feature request related to a problem?

TextMapCarrier requires all implementations to provide both Injector and Extractor behavior from the OpenTelemetry specification. This is unnatural from the C++ perspective and not compatible with some communication frameworks.

Describe the solution you'd like Separate TextMapCarrier into Injector and Extractor interfaces. Update TextMapPropagator to use different interfaces for the Inject and Extract methods.

Describe alternatives you've considered Assuming the caller has some container const T t on which they would base a TextMapCarrier implementation, the naive solutions involve either const_casting or copying t. The former creates surface area for correctness issues while the latter incurs a performance penalty.

The TextMapCarrier implementation could provide Set as a no-op which avoids the performance penalty, but still carries correctness risks. A misuse of this solution could be expensive to identify (especially given that Set is marked noexcept).

The TextMapCarrier implementation could use a copy-on-write container. This solution has additional complexity, either requiring the client to provide such a container or requiring the client to understand the additional constraints of an OpenTelemetry-provided solution.

Separation of interfaces seems to provide the best trade-off, including:

• Allowing the consumer to more precisely express intent which is enforceable by the compiler;
• Creating a simple API with idiomatic basis in the C++ standard (and explicit allowance in the OpenTelemetry specification); and
• Incurring the lowest performance penalty for the required scope.

[marcalff]

Thanks for the issue.

I experienced the same problem myself, having to implement a TextMapCarrier with no op and assert(false) for one interface, implementing only the other one.

Not sure how it can be fixed without affecting the existing API, and without causing breaks, but this is a valid concern in my opinion.

[lalitb]

Agree, this is valid concern. Thanks for raising it. However, as said above, don't see how can we implement it without breaking existing API.

[kyleholohan]

I can think of a somewhat hacky way to do it in the library:

• Add TextMapExtractor/Injector with only the Extract/Inject functionality
• Add Inject/Extract overloads to TextMapPropagator taking the appropriate new interface
• Add default implementations to the new TextMapPropagator::Inject/Extract overloads which wrap the e.g. TextMapExtractor in some kind of adapter class derived from TextMapCarrier, then call the existing pure virtual overload.

e.g. an abridged implementation:

class TextMapPropagator {
// ...
  virtual context::Context Extract(const TextMapExtractor &extractor, ...) {
      struct Adapter : TextMapCarrier {
          TextMapExtractor &underlying;
          virtual ... Get(...) ... {
               return underlying.Get(...);
          }
          virtual ... Set(...) ... { }
      };
      return Extract(Adapter{ extractor });
    }

Inject follows with some minor changes.

It's a bit awkward and is essentially one of the alternatives above (i.e. implement TextMapCarrier with a no-op Get or Set), but the tight scope at least allows for verifiability.

Edit: This solution is additive, so I guess it depends whether you consider and additive API change to be breaking.

[sirzooro]

Did not try to compile this, but it should allow to keep API compatibility:

class TextMapCarrierKeys
{
public:
  virtual bool Keys(nostd::function_ref<bool(nostd::string_view)> /* callback */) const noexcept {  return true; }
  virtual ~TextMapCarrierKeys() = default;
};

class TextMapCarrierInjector : public virtual TextMapCarrierKeys
{
public:
  virtual void Set(nostd::string_view key, nostd::string_view value) noexcept = 0;
};

class TextMapCarrierExtractor : public virtual TextMapCarrierKeys
{
public:
  virtual nostd::string_view Get(nostd::string_view key) const noexcept = 0;
};

class TextMapCarrier : public TextMapCarrierInjector, public TextMapCarrierExtractor
{
};

Additionally you need to change methods in TextMapPropagator to accept one of base interfaces instead of TextMapCarrier .

[lalitb]

Thanks @sirzooro , @apelyk . I think we still have linking issues with the instrumentation libraries build against the otel-api using the older interface for TextMapCarrier, and the application compiled against the otel-sdk with new TextMapCarrier interface ? We do guarantee the ABI compatibility for otel-api interface.

[sirzooro]

Yes, my solution is not ABI-compatible. It would require recompilation of user code.

I am not sure if new virtual methods is ABI compatible. If yes, @apelyk's solution would be better for now - add new interfaces and overloads for them in TextMapPropagator. My solution could be added later, when ABI compatibility is intentionally not kept.

[marcalff]

From the spec:

" Injectors and Extractors as Separate Interfaces

Languages can choose to implement a Propagator type as a single object exposing Inject and Extract methods, or they can opt to divide the responsibilities further into individual Injectors and Extractors. A Propagator can be implemented by composing individual Injectors and Extractors. "

[sirzooro]

One more thing to consider when splitting these interfaces: I assumed that Keys method is needed for both of them. If it is used by one of them only, move it there and remove base interface class TextMapCarrierKeys.

[github-actions[bot]]

This issue was marked as stale due to lack of activity.