Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.19%. Comparing base (700227a) to head (acbbbaf).
⚠️ Report is 8 commits behind head on main.

@@             Coverage Diff              @@
##               main    #7819      +/-   ##
============================================
- Coverage     90.20%   90.19%   -0.02%     
- Complexity     7245     7248       +3     
============================================
  Files           822      822              
  Lines         21828    21832       +4     
  Branches       2139     2141       +2     
============================================
+ Hits          19691    19692       +1     
- Misses         1469     1471       +2     
- Partials        668      669       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I was thinking about this in the context of #6604.

This functionality unlocks a potential recommendation strategy that is currently not possible: Native instrumentations should default to GlobalOpenTelemetry if it is set, else use a noop. Native instrumentations should expose an API to explicitly configure the OpenTelemetry instance.

A typical builder for some client that is natively instrumented might look like:

public class MyClientBuilder {
  private OpenTelemetry openTelemetry = GlobalOpenTelemetry.isSet() ? GlobalOpenTelemetry.get() : OpenTelemetry.noop();
  // .. other properties omitted

  public MyClientBuilder setOpenTelemetry(OpenTelemetry openTelemetry) {
    this.openTelemetry = openTelemetry;
  }

  // ... other setters omitted

  public MyClient build() {
    return new MyClient(openTelemetry, /* other properties omittted */);
  }

Not bad. This sidesteps the issues we've struggled with in the java SIG with the javaagent trying to detect native instrumentations and automatically install the configured OpenTelemetry instance. Instead, native instrumentations work by default when the agent is present because GlobalOpenTelemetry.isSet() returns true.

We still have the potential for double instrumentation if both native and auto instrumentation is present. But let's ignore that for a second.

If we like the strategy I laid out above, I wonder if we can design an API for GlobalOpenTelemetry which acheives the same affect but is a little cleaner.

One alternative:

• Introduce @Nullable OpenTelemetry getIfSet(), which returns null if GlobalOpenTelemetry.set() has not been called.
• Native instrumentation default code looks something like:

public class MyClientBuilder {
  private OpenTelemetry openTelemetry = Objects.requireNonNullElse(GlobalOpenTelemetry.getIfSet(), OpenTelemetry.noop());

@open-telemetry/java-instrumentation-approvers what do you think? Is this a viable strategy for guidance to native instrumentations? Are there other ideas for GlobalOpenTelemetry API additions that achieve the same affect but allow our guidance to be cleaner?

Previously I tagged go approvers instead of java instrumentation approver. This was a typo. Sorry!

maybe take one step further?

public class MyClientBuilder {
  private OpenTelemetry openTelemetry = GlobalOpenTelemetry.getOrNoop();

maybe take one step further?

public class MyClientBuilder {
  private OpenTelemetry openTelemetry = GlobalOpenTelemetry.getOrNoop();

That seems overly restrictive - maybe someone wants to fallback to something else than noop

getIfSet has the advantage that you don't need to think about what happens if the instance is set after calling isSet, because it's a single call

One other use case which is solved by this concept is: an application owner, who wants to install custom instrumentation, where the OpenTelemetry javaagent is sometimes used, and sometimes not. They want to record their custom instrumentation to the javaagent's OpenTelemetry instance when its set, and initialize their own instance when its not.

With isSet:

OpenTelemetry openTelemetry = GlobalOpenTelemetry.isSet() ? GlobalOpenTelemetry.get() : initializeOpenTelemetry();

With getIfSet:

OpenTelemetry openTelemetry = Optional.ofNullable(GlobalOpenTelemetry.getIfSet()).orElseGet(() -> initializeOpenTelemetry());

With getOrNoop:

OpenTelemetry openTelemetry = GlobalOpenTelemetry.getOrNoop();
if (openTelemetry == OpenTelemetry.noop()) {
  openTelemetry = initializeOpenTelemetry();
}

Will merge on 11/13 if there is no other thoughts / feedback on ergonomics.

One last plug for getOrNoop() as it makes advice for native instrumentation simpler: #6604 (comment)

One last plug for getOrNoop() as it makes advice for native instrumentation simpler: #6604 (comment)

I'm also fine with that - reverting my earlier comment 😄

@trask improves native instrumentation advice, but makes it worse for application application owners installation custom instrumentation:

OpenTelemetry openTelemetry = GlobalOpenTelemetry.getOrNoop();
if (openTelemetry == OpenTelemetry.noop()) {
  openTelemetry = initializeOpenTelemetry();
}

#7819 (comment)

(I don't have a strong opinion here. Either approach works. getOrNoop is more ergonomic for native instrumentation. isSet() is slightly more ergonomic for application owners. Both are slightly confusing and will likely require reading the docs to get right.)

maybe two methods?

• getOrNoop()
• getOrInstall(Supplier)

I can get behind that. I would name it getOrSet(Supplier) for consistency.

Pushed a commit to add getOrNoop or getOrSet instead. Updated PR description. Cleared approvals since this is materially different.

Writing this, I realized that with these changes, GlobalOpenTelemetry#get() starts to look odd and very niche. In fact, I can't think of who I would recommend using this with these new additions. It does have the option to trigger autoconfiguration, which is interesting I guess, but still not sure who its for.

Additionally, all the getTracerProvider(), getTracer(...) style helper methods which call get() (and therefore have its side affects) start to look like real foot guns we should consider deprecating and warning against using.

Writing this, I realized that with these changes, GlobalOpenTelemetry#get() starts to look odd and very niche. In fact, I can't think of who I would recommend using this with these new additions. It does have the option to trigger autoconfiguration, which is interesting I guess, but still not sure who its for.

End users in Java agent would call get

End users in Java agent would call get

With the additions in this PR, are those callers better off using get() than getOrNoop?

Writing this, I realized that with these changes, GlobalOpenTelemetry#get() starts to look odd and very niche. In fact, I can't think of who I would recommend using this with these new additions. It does have the option to trigger autoconfiguration, which is interesting I guess, but still not sure who its for.

I had the same thought, and I like the outcome, since get() is very hard to reason about anyways, and so this potentially gives us an offramp from it

End users in Java agent would call get

With the additions in this PR, are those callers better off using get() than getOrNoop?

It doesn't matter - but "get or noop" just feels too verbose

hmm. Not sure I prefer this option over just having "isSet()", which seems simpler, overall, even if you have a couple extra lines of code when you want to use it. I don't like the "getOrNoop" then compare to Noop pattern, really. That feels fragile and harder to explain than just having a simple isSet() method and let the user decide what they want to do with that information.

I don't like the "getOrNoop" then compare to Noop pattern, really.

I think the idea is that you should never have to compare with Noop - and therefore Noop can be obfuscated.
The use case where you'd want to compare with Noop is covered by the new getOrSet method.

I don't like the "getOrNoop" then compare to Noop pattern, really.

I think the idea is that you should never have to compare with Noop - and therefore Noop can be obfuscated. The use case where you'd want to compare with Noop is covered by the new getOrSet method.

Sure, but it only then gives you the option of setting one, you can't do anything else if it hasn't been set yet. So, I don't know that all use cases are actually solved here... I think we'll have to end up adding isSet to facilitate them, and hence I think we should just favor that.

Sure, but it only then gives you the option of setting one, you can't do anything else if it hasn't been set yet. So, I don't know that all use cases are actually solved here... I think we'll have to end up adding isSet to facilitate them, and hence I think we should just favor that.

Yeah.. both getOrNoop and getOrSet are syntactic sugar for what we think will be the most common use cases of isSet.

getOrNoop is equivalent to:

return isSet() ? get() : noop();

getOrSet is equivalent to (but notably holds the mutex the whole time so no concurrent interaction GlobalOpenTelemetry can impact the outcome):

if (isSet()) {
  return get();
} else {
  set(supplier.get())
}

I do think the getOrNoop is nice syntactic sugar, and will be used heavily enough to warrant.

Not so sure about getOrSet. I mean I don't dislike getOrSet, but I suspect there will be cases where a caller would prefer to just have isSet and would feel like being to do the equivalent via getOrSet is needlessly cumbersome.

Options:

1. Only isSet

• Simple
• Native / java agent use case will want the getOrNoop syntactic sugar

2. getOrNoop and getOrSet

• Need more docs to explain what is appropriate to use where
• more cumbersome than isSet for certain cases

3. getOrNoop, getOrSet, and isSet

• Most flexible
• Most API surface area

4. getOrNoop and isSet

• Syntactic sugar for most popular native / java agent use case
• Slightly more verbose for custom instrumentation use case

I'm leaning towards 4, but would be fine with 3 as well. I think if start at 1, we'll find ourselves frustrated with having to replace a ton of calls in opentelemetry-java-instrumentation from get to isSet() ? get() : noop() and will end up providing getOrNoop anyway.

4 seems like a nice balance.

4 seems like a nice balance.

👍

I'm fine with option 4. I don't quite grok why the agent wants to have getOrNoop, but I'm not opposed to it being there if it will help.

I don't quite grok why the agent wants to have getOrNoop

it's to simplify the native instrumentation story: #6604 (comment)

I don't quite grok why the agent wants to have getOrNoop

it's to simplify the native instrumentation story: #6604 (comment)

ah, ok, I think I understand that use-case. Won't we need to advise people to not keep a reference to a potential no-op instance, then, in case they ask for their first instance before the global gets initialized? I guess if you're running the agent, that's going to be super-rare, since it gets initialized in pre-main, but what about non-agent cases where it definitely would be possible for the global to be set after someone has gotten a Noop instance from the new method?

but what about non-agent cases where it definitely would be possible for the global to be set after someone has gotten a Noop instance from the new method?

Its possible, but we have competing concerns.

The native instrumentation story needs a good java agent interop story by default. After multiple discussions in the java SIG, we we're able to find a good way for the agent to efficiently automatically detect native instrumentations and install its own instance. So instead we flip the script and tell native instrumentations to call getOrNoop as a means of getting the java agent instance when installed.

In a way this is at odds with our current guidance / implementation of GlobalOpenTelemetry.get(), where we say it atomically always returns the same instance. But in the java agent case, getOrNoop is a little different because java agent initialization will always occur before native instrumentation can call getOrNoop.

So if we're operating without the java agent, and a user calls GlobalOpenTelemetry.set() after a native instrumentation calls getOrNoop, then we arguably have the "hard to debug initialization issues". But, its different because the getOrNoop native instrumentation advice isn't meant to allow applications install their own instance into native instrumentations. Its only meant for the java agent.

This route is definitely a shift in our framing about GlobalOpenTelemetry.

But we've got to do something because our current advice has a terrible native instrumentation + java agent interop story.

Updated PR to reflect option 4. Can always add syntactic sugar getOrSet later if we get user feedback that isSet() isn't enough and that you need to be able to call a supplier while holding the mutex.