ScopedValue.runWhere not returning scope
Robert Engels
robaho at icloud.com
Thu Jun 13 15:03:21 UTC 2024
Marcin,
A few of things:
1. If you look at the javadoc at the top of CurrentTraceContext.java:
"This makes a given span the current span by placing it in scope (usually but not always a thread local scope).”
and then farther down in the Default class:
"static thread local ensures we have one context per thread, as opposed to one per thread-tracer. This means all tracer instances will be able to see any tracer's contexts.
The trade-off of this (instance-based reference) vs the reverse: trace contexts are not separated by tracer by default. For example, to make a trace invisible to another tracer, you have to use a non-default implementation."
It is an implementation detail whether or not a ThreadLocal is used. So for Scoped tracers, it may be that the easiest implementation is “private” traces. Whether or not a system is ok with private tracers is system dependent.
2. More importantly, If every thread execution VT or PT has a ScopedValue<SomeRef<X>> Y available, it is trivial to replace ThreadLocal<X> with Y.get().set(x), and Y.get().get().
Given the above it is straightforward to create a concrete implementation of CurrentTraceContext that uses a ScopedValue rather than ThreadLocal. The only other change to the framework would be to ensure that every Thread created sets up the scope variable X, in order to allow shared tracers using ScopedValues.
Which goes back to my original point - if the system authors are re-architecting to use VirtualThreads effectively, they can make the minor changes required to the “thread starting” required by their chosen tracing library. It is most likely a single line code change.
3. More importantly, if you use VT effectively, you are talking about orders of magnitude more “trace contexts” (in the default brave handler they are shared by thread so they are highly limited since the only effective Java threading implementation prior to VT required pooled threads) which reinforces my point about the increase in the volume of unique data is the major problem for existing tracing libraries, which will probably require architectural changes anyway.
4. Lastly, as has been pointed out, TL are not going away, so the existing libraries will “work” - except possibly fail given point 3.
> On Jun 13, 2024, at 4:48 AM, Marcin Grzejszczak <marcin.grzejszczak at gmail.com> wrote:
>
> Robert, in that exact class that you're showing (the `ThreadLocalCurrentTraceContext` [1] ) you have the method
>
> @Override public Scope newScope(@Nullable TraceContext currentSpan) {
> final TraceContext previous = local.get();
> local.set(currentSpan);
> Scope result = previous != null ? new RevertToPreviousScope(local, previous) : revertToNull;
> return decorateScope(currentSpan, result);
> }
>
> That very class extends the CurrentTraceContext that requires to define how you create a new Scope. How will that work with Scoped Values?
>
> [1] - https://github.com/openzipkin/brave/blob/6.0.3/brave/src/main/java/brave/propagation/ThreadLocalCurrentTraceContext.java <https://github.com/openzipkin/brave/blob/6.0.3/brave/src/main/java/brave/propagation/ThreadLocalCurrentTraceContext.java>
>
> Pozdrawiam / Best regards,
> Marcin Grzejszczak
>
> https://marcin.grzejszczak.pl <https://marcin.grzejszczak.pl/>
> https://toomuchcoding.com <https://toomuchcoding.com/>
>
> śr., 12 cze 2024 o 12:55 robert engels <robaho at icloud.com <mailto:robaho at icloud.com>> napisał(a):
> Also, looking at brave, they seem to understand the various ways of managing the current context. See https://github.com/openzipkin/brave/tree/master/brave/src/main/java/brave/propagation <https://github.com/openzipkin/brave/tree/master/brave/src/main/java/brave/propagation>
>
> and so plugging into this a context based on a ScopedValue seems trivial (see notes in TraceContextThreadLocal)
>
> I would expect other libraries have similar features.
>
>> On Jun 12, 2024, at 7:17 AM, robert engels <robaho at icloud.com <mailto:robaho at icloud.com>> wrote:
>>
>>
>>
>>
>>
>>
>> The code I provided will also work with before/after only tracers - the around is a simplification of this.
>>
>> Wrapping of the Runnable would happen the same way all instrumentation is done: manually, byte code manipulation, or dynamic proxies.
>>
>> The SV vs TL is only how the current context is obtained. It is trivial to change between the two.
>>
>> I think it would be helpful if you created a project that uses the aspects that demonstrates your perceived problem rather than the abstract solutions. This feels like an XY problem.
>>
>> Or do nothing, TLs are not going away.
>>
>>
>>
>>
>>> On Jun 12, 2024, at 4:07 AM, Marcin Grzejszczak <marcin.grzejszczak at gmail.com <mailto:marcin.grzejszczak at gmail.com>> wrote:
>>>
>>>
>>> > I posted a minimalist “tracing library” using ScopedValue here https://github.com/robaho/scope_trace <https://github.com/robaho/scope_trace>
>>>
>>> Thanks for doing the example.
>>>
>>> >I think it is a fairly trivial change for tracing libraries to support ScopeValue instead of ThreadLocal if they wished to.
>>>
>>> As I presented by showing AFAIR 6 projects, not every single time the change would be trivial. Not all libraries give you the option to instrument the actual runnable (like an around aspect). Sometimes you have only before and after. Again, let me emphasise, I'm not saying that changing these libraries would not be possible. I'm saying that it wouldn't always be trivial and would require changes in various different projects.
>>>
>>> Pozdrawiam / Best regards,
>>> Marcin Grzejszczak
>>>
>>> https://marcin.grzejszczak.pl <https://marcin.grzejszczak.pl/>
>>> https://toomuchcoding.com <https://toomuchcoding.com/>
>>>
>>> wt., 11 cze 2024 o 15:25 Robert Engels <robaho at icloud.com <mailto:robaho at icloud.com>> napisał(a):
>>> I posted a minimalist “tracing library” using ScopedValue here https://github.com/robaho/scope_trace <https://github.com/robaho/scope_trace>
>>>
>>> I think it is a fairly trivial change for tracing libraries to support ScopeValue instead of ThreadLocal if they wished to.
>>>
>>>
>>>> On Jun 11, 2024, at 7:35 AM, Andrew Haley <aph-open at littlepinkcloud.com <mailto:aph-open at littlepinkcloud.com>> wrote:
>>>>
>>>> On 6/11/24 11:32, Marcin Grzejszczak wrote:
>>>>
>>>> >> So why not produce an example of how it would be used? As far as I can
>>>> >> see, your proposal fails to ensure the property in the case I posted/
>>>> >
>>>> > I've created samples at the very beginning with the before and after
>>>> > interceptor. I obviously know that I could rewrite the interceptor
>>>> > to become an around one but my point is that it's not an ideal
>>>> > solution to require instrumentors to rewrite everything. Also I
>>>> > still see this whole discussion as having two ways of doing the same
>>>> > thing. Current approach:
>>>> >
>>>> > ScopedValue.runWhere(scopedValue, someValue, () -> {
>>>> > codeToRun();
>>>> > });
>>>> >
>>>> > Proposed approach:
>>>> >
>>>> > try(Scope scope = ScopedValue.openScope(scopedValue, someValue)) {
>>>> > codeToRun();
>>>>
>>>> This API fails the test I provided:
>>>>
>>>> final ScopedValue aScopedValue = ...;
>>>> final var x = aScopedValue.get();
>>>> ScopedValue.openScope(aScopedValue, someValue);
>>>> final var y = aScopedValue.get();
>>>>
>>>> assert(x == y); // Always succeeds
>>>>
>>>> In other words, scoped values would no longer be strictly scoped.
>>>> That's what the "scoped" means in the name "scoped values". Strict
>>>> scoping is a fundamental property.
>>>>
>>>> > }
>>>> >
>>>> > I understand that in the proposed approach one can mess things up, I
>>>> > really do. But library instrumentors and tracing library creators
>>>> > would benefit from that sort of API.
>>>>
>>>> If we were planning to deprecate thread-local variables I think this
>>>> argument might have some merit. But we're not. They're not going away.
>>>>
>>>> > Also I've asked a different question... What if the users want to
>>>> > leverage the ScopedValues to get the current span information (or
>>>> > current principal from security point of view) but also they are
>>>> > using TL based libraries? How would the interop look like?
>>>>
>>>> As the JEP says, scoped values do not replace all uses of thread-local
>>>> variables. There are still some cases where thread-local variables
>>>> will be used, and this looks like one of them.
>>>>
>>>> > Let's assume that they put into the current scope a span that they
>>>> > want to have within that scope, we would need all the tracer
>>>> > libraries (and security components and others that use the same
>>>> > mechanism) to be ScopeValue aware. Would they first try to read SV
>>>> > and then if that's not there a TL?
>>>>
>>>> Yes, I think so.
>>>>
>>>> > They would need to most likely read SV and populate TL if the SV
>>>> > value is present so that other components that still rely on TL
>>>> > would work. That would have to also work the other way round, if a
>>>> > TL based library sets a value (e.g. span) then maybe the ScopedValue
>>>> > should be created with a scope for that new value?
>>>>
>>>> I'm not sure how that'd work in general, but there's nothing to stop
>>>> entry points for a library from doing so, and it might be a good idea.
>>>> Something like:
>>>>
>>>> if (someThreadLocal.get() != NOTHING) {
>>>> where(someScopedValue, someThreadLocal.get()).run(realEntry);
>>>> } else {
>>>> realEntry.run();
>>>> }
>>>>
>>>> --
>>>> Andrew Haley (he/him)
>>>> Java Platform Lead Engineer
>>>> Red Hat UK Ltd. <https://www.redhat.com <https://www.redhat.com/>>
>>>> https://keybase.io/andrewhaley <https://keybase.io/andrewhaley>
>>>> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
>>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/loom-dev/attachments/20240613/fd7ac8e0/attachment-0001.htm>
More information about the loom-dev
mailing list