[API] Make Request Context Token constructor public

[aknott]

Fixes #3034

I'd like to implement a custom RuntimeContextStorage and need to store some additional data in a subclass of Token. Based on previous discussions in #3034, it seemed like people were OK with making this constructor public, so I'm doing that here.

Changes

• Moves Token constructor from private to protected
• Makes context_ protected, so subclasses can access it.
• Makes Token destructor virtual

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: marcalff / name: Marc Alff (e5b561c)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.00%. Comparing base (33029bc) to head (e5b561c).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3708      +/-   ##
==========================================
- Coverage   90.01%   90.00%   -0.01%     
==========================================
  Files         225      225              
  Lines        7105     7105              
==========================================
- Hits         6395     6394       -1     
- Misses        710      711       +1     

Files with missing lines	Coverage Δ
...pi/include/opentelemetry/context/runtime_context.h	92.14% <ø> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[marcalff]

LGTM, thanks for the fix.

[marcalff]

@lalitb @ThomsonTan @esigo

Please review.

This looks good to me, but given how this touches the very core of the instrumentation API (runtime context), better be safe than sorry.

[lalitb]

Not ABI-breaking, but the change looks unnecessary and could introduce design issues by increasing visibility. If additional data needs to be associated, it can be handled externally (e.g., std::unordered_map<const Token*, MyData>) or through a wrapper. Better to keep Token private and opaque.

[marcalff]

Not ABI-breaking, but the change looks unnecessary and could introduce design issues by increasing visibility. If additional data needs to be associated, it can be handled externally (e.g., std::unordered_map<const Token*, MyData>) or through a wrapper. Better to keep Token private and opaque.

How would this work ?

The whole point is to inherit form Token, to trigger custom code in the destructor, invoked when the unique_ptr gets out of scope.

With a separate map, when would the map be maintained ?

[marcalff]

Note that Token itself is still private and opaque, what is needed is to subclass it in a custom runtime context.

[lalitb]

The whole point is to inherit form Token, to trigger custom code in the destructor, invoked when the unique_ptr gets out of scope.

Maybe I am missing something - what's the use-case of inheriting from Token.

[aknott]

I'm specifically trying to integrate folly::RequestContext as the underlying storage for context. Ideally this is done by attaching the folly::ShallowCopyRequestContextScopeGuard within a derived Token class. When that goes out of scope, folly::RequestContext handles restoring previous context automatically.

[Reneg973]

At least the destructor should be virtual then.
And, if it's a custom token, the Token's == operator may not be good enough, so == may also get virtual?

[marcalff]

@lalitb

Let's discuss the details in the next opentelemetry-cpp SIG meeting:

• how to implement a custom runtime context
• how to add custom data with a token, and retrieve it during Token destruction or ::Detach().

[lalitb]

@lalitb

Let's discuss the details in the next opentelemetry-cpp SIG meeting:

• how to implement a custom runtime context

• how to add custom data with a token, and retrieve it during Token destruction or ::Detach().

@marcalff will try to join the meeting bit late. To summarise my concern

• would good to be conservative in making constructs publicly visible unless not doing so breaks the basic functionality.
• if we can't create custom storage runtime without this fix, the change is fine.
• I am not sure if we should allow the token to be inherited, and so add custom data added. Would review the use case more thoroughly today, and reply back.

[lalitb]

Can we keep

Token(const Context &context) : context_(context) {}

as protected as well ?

[lalitb]

LGTM, I think there are workaround by storing guards in RuntimeContextStorage, but the change achieves better RAII pattern.

[ThomsonTan]

Instead of making it public, is protected sufficient to make Token constructible in your subclass of Token, which then can be used in the subclass of RuntimeContextStorage? This way, we still don't expose the Token class to public.

[marcalff]

Looks ok, with the following changes:

• The constructor of Token should be protected, not public
• The destructor of Token should be virtual

[Reneg973]

Looks ok, with the following changes:

• The constructor of Token should be protected, not public
• The destructor of Token should be virtual

And, as the virtual destructor implies, the custom token may have its own state, it may also require a custom ==. Therefore

• make operator== virtual

[marcalff]

And, as the virtual destructor implies, the custom token may have its own state, it may also require a custom ==. Therefore

* make operator== virtual

@Reneg973

Thanks for the comments.

In this case, the CustomToken subclass is used to keep state that precisely is not stored into the Context, otherwise the plain Token class should be used in the first place.

The operator== is to compare with a Context, not a token in general, so I don't think this equality operator should be virtual (but it is possible I missed something).

Of course it would be better to see an actual implementation of a CustomToken and a CustomRuntimeContextStorage to grasp all what is needed here, I think what is missing the most is an example custom runtime context to validate we have all the missing pieces here, as well as show a template implementation to use, but we don't have that at the moment.

[Reneg973]

In this case, the CustomToken subclass is used to keep state that precisely is not stored into the Context

Yes, you're right. Somehow I've overseen that it's not an equality operator. In this case the operator is a bit abused ;).
If a token 'is a' context, something like using Token = Context; would be easier

[marcalff]

LGTM