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

Make it possible to use InstrumentationContext (now VirtualField) fro… #4218

Merged

Conversation

mateuszrzeszutek
Copy link
Member

…m library instrumentation

Resolves #4081

  • Renamed ContextStore to VirtualField in public APIs
  • Moved & renamed InstrumentationContext#get() to VirtualField#find()
  • Moved VirtualField to intstrumentation-api

There's still several mentions of ContextStore/InstrumentationContext remaining, but they're all located somewhere in the agent internals (or muzzle). I'll rename them in a follow-up PR (once muzzle InstrumenterConfigBuilder PR gets merged)

*
* @return The old field value if it was present, or the passed {@code fieldValue}.
*/
public abstract F setIfAbsentAndGet(T object, F fieldValue);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps call it just setIfAbsent

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted this method to use a similar wording to AtomicReference/VarHandle methods, even if it's a bit explicit.

public abstract void set(T object, @Nullable F fieldValue);

/**
* Sets the new value of this virtual field if the current value is {@code null} or absent.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove the or absent part as we really only check whether the field is null

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.
Now I'm starting to wonder whether the method should be named setIfNullAndGet...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like setIfNull

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was thinking setIfNull without the AndGet, similar to Map.putIfAbsent, but now i realize the potential confusion because putIfAbsent returns the previous value or null if there was none, while this method always returns the stored value.

maybe:

void setIfNull(T object, F fieldValue)

and

F computeIfNull(T object, Supplier<F> fieldValueSupplier)

as it looks like we don't (currently at least) use the return value from the first method, though now I'm wondering if that method is even needed, i.e. will anything break if those callers just use set (and if the "set if null" behavior really does matter, they could call get first, as there's nothing atomic anyways if they aren't using the return value)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

though now I'm wondering if that method is even needed, i.e. will anything break if those callers just use set (and if the "set if null" behavior really does matter, they could call get first, as there's nothing atomic anyways if they aren't using the return value)

Most of the setIfNull usages look like this method is used instead of plain set "just in case something happens"; even though there seems to be no real chance of the field being non-null. I can take a look at them and maybe remove the method in a follow-up PR.

/**
* Represents a "virtual" field of type {@code F} that is added to type {@code T} in the runtime.
*
* <p>Field values are weakly referenced and will be garbage collected when their owner instance is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that field values are not weakly referenced, if they were they could be collected if nobody else has a reference to them.
If I understand this correctly the happy path is that we transform the class and add a field to it so we really have a regular strong reference to the value. Things get messy when class is not transformed for some reason. Then we put the values into a weak map where key is the object on which field is set. This has the annoying property that when value references key then we immediately have a memory leak because now key can be never collected. We could get around this by also making values weak which would mean that something else needs to keep the values alive. Basically afaik no matter what we do we can't get the same behaviour as when the value is just stored in a field.
Perhaps we could address this somehow. Maybe we could drop the support for using fallback map? Now that lambda classes are also transformed is there a case where it could be needed? There is a flag to disable field injection and there is hack that skips filed injection on classes annotated with javax.decorator.Decorator so might not be that easy to completely get rid of it. An alternative would be to have something in VirtualField api that lets user figure out whether it is field or map based. Or could add methods like setWeak that doesn't change anything in field backed implementation but for map based implementation makes the value also weak. This would allow using VirtualField in netty FutureListenerWrappers without leaking memory even when map based store is used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that field values are not weakly referenced, if they were they could be collected if nobody else has a reference to them.

Yeah that's true, I copied this Javadoc and didn't really think too deeply about it.

Basically afaik no matter what we do we can't get the same behaviour as when the value is just stored in a field.
Perhaps we could address this somehow. Maybe we could drop the support for using fallback map?

We probably have to keep the fallback implementation; I suppose it's better to have it in place and install a few more instrumentations than not to have it and skip instrumenting some pieces of code.

Perhaps we could fix the Javadoc so that it outright says that this has similar semantics to a weak-keys strong-values map and keeping values that might reference the original key is discouraged?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could drop the support for using fallback map?

that would be nice, the memory leak has bit us before (at least #441 and #3027 I think)

Now that lambda classes are also transformed is there a case where it could be needed? There is a flag to disable field injection and there is hack that skips filed injection on classes annotated with javax.decorator.Decorator so might not be that easy to completely get rid of it

I wonder if the javax.decorator.Decorator is needed since #3948

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the original DD PR that introduced this mathcer: DataDog/dd-trace-java#1045

In CDI (Contexts and Dependency Injection) environments, classes annotated with @Decorator wrap other classes.
Decorators implement the same interfaces but delegate to another class. See the Oracle docs.

In general, the tracer should instrument the delegate (decorated class) and not the decorator. When the tracer instruments the decorator, it cause the CDI container to be confused about the interfaces of the decorator and crash on startup.

If we can reproduce that problem and verify that #3948 solves it then we can start thinking of phasing out the weak map implementation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opened #4241 to track/discuss further

Comment on lines -313 to +315
ContextStore<Runnable, Context> contextStore =
InstrumentationContext.get(Runnable.class, Context.class);
VirtualField<Runnable, Context> virtualField =
VirtualField.get(Runnable.class, Context.class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ❤️ reducing ContextStore and InstrumentationContext down to just one concept VirtualField (and that doesn't have the word Context in it)

Comment on lines +15 to +18
public static VirtualField<Connection, DbInfo> connectionInfo =
VirtualField.find(Connection.class, DbInfo.class);
public static VirtualField<PreparedStatement, String> preparedStatement =
VirtualField.find(PreparedStatement.class, String.class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

/**
* Represents a "virtual" field of type {@code F} that is added to type {@code T} in the runtime.
*
* <p>Field values are weakly referenced and will be garbage collected when their owner instance is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could drop the support for using fallback map?

that would be nice, the memory leak has bit us before (at least #441 and #3027 I think)

Now that lambda classes are also transformed is there a case where it could be needed? There is a flag to disable field injection and there is hack that skips filed injection on classes annotated with javax.decorator.Decorator so might not be that easy to completely get rid of it

I wonder if the javax.decorator.Decorator is needed since #3948

@mateuszrzeszutek mateuszrzeszutek merged commit c11b96e into open-telemetry:main Oct 1, 2021
@mateuszrzeszutek mateuszrzeszutek deleted the virtual-fields branch October 1, 2021 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simpler solution for library instrumentation to benefit from InstrumentationContext when used in agent
3 participants