Only construct Observation.Context for Chat and Vector ops on Model observations

[jxblum]

Many of the AI provider ChatModels prematurely construct an Observation.Context when ChatModel Observations may not even be enabled (as defined by Micrometer in ObservationDocumentation and Observation).

• Simplify and polish code in OpenAI using the API with Spring constructs and Builders.
• Simplify common code expressions used in AI provider ChatModels with ValueUtils.
• Apply whitespace to improve readability.

[markpollack]

I'm a bit nervous to get this in before Nov 11, but nevertheless do want to know about best practices here - there are many it seems that are a bit opaque to me. How expensive is it to create an ObservationContext? @ThomasVitale @jonatan-ivanov ?

[jonatan-ivanov]

How about calling it observationContextSupplier? (There are a few other occasions.)

The point of passing the context via a Supplier when you create an Observation is exactly this; when observations are "off" (noop), it will not call the supplier so no unnecessary context objects will be created. 👍🏼

In this particular case though, since the request can be different for every call, this replaces creating a new context to creating a new Supplier which I think might be more lightweight but there is a bit of additional complexity through the noop check + insanceof + cast + using Optional so which one is more lightweight, I'm not sure, only JMH can tell I guess.

[jxblum]

I am fine with whatever name we use. In some cases (such as Wrapper types or Wrapper-like types, e.g. Supplier, or Optional), I simply use, or prefer, the name of the thing it wraps.

My reasoning is also similar in effect to List<User> users vs. List<User> userList. I like users, particularly if the collection-type might change (e.g. Set)

[jonatan-ivanov]

There is an isNoop, when would null be passed to this method?
This might be simplified as:

public static Optional<ChatModelObservationContext> getObservationContext(Observation observation) {
    return observation.isNoop() : Optional.empty() ? Optional.of((ChatModelObservationContext) observation.getContext());
}

or

public static <T> Optional<T> getObservationContext(Observation observation) {
    return observation.isNoop() : Optional.empty() ? Optional.of((T) observation.getContext());
}

[jxblum]

Honestly, I went back and forth on this.

I realize that in the context of Micrometer, the Observation is likely to never to be null. But, arguably, contracts can change (or break). So, over many years of systems development, I learned to code defensively... assume the worse, fail gracefully and deliberately, not unexpectedly.

GENERAL THINKING:

This will never fail (providing the equals(..) method is coded correctly), where as, object.someMethod(..) can throw a potential NPE at runtime if our library is not used properly, even despite the "nullability" contract. Additionally, the "nullability" contract is not always apparent (despite implied or explicit annotations, which rely on tooling to do the right thing, e.g. warn the user). The support class does not make any assumptions about how the Observation was created (e.g. with a ObservationDocumentation, or otherwise, or even at all). The point is, we don't know how our method will be called.

This same principle applies to methods that return arrays or collections. It is a recommended practice for classes to never return a null array or collection, but rather an empty one. Still, I never take that for granted and assume a 3rd party library (or even my own code; mistakes happen) will always do the right thing, so I guard against the possibility of null arrays/collections.

For example:

List<User> users = userService.findByName("...");
nullSafeList(users).forEach(user -> ...);

Where:

static <T> List<T> nullSafeList(List<T> list) {
  return list != null ? list : Collections.emptyList();
}

Rather than:

List<User> users = userService.findByName("...");
users.forEache(user -> ...);

The later is bad if the contract of UserService.findByName(..):List<User> is broken, returning null instead of empty.

This might seem like paranoia, until it isn't.

Safety first.

[jonatan-ivanov]

You can do this instead (might be simpler):

if (!observation.isNoop()) {
    ((ChatModelObservationContext)observation.getContext()).setResponse(chatResponse);
}

[jxblum]

I generally agree, but I'd like to see something like this in Micrometer:

interface Observation {

  static boolean isNoop(Observation observation) {
    return observation == null || observation.isNoop();
  }

  static boolean isNotNoop(Observation observation) {
    return !isNoop(observation);
  }

  // ...
}

[jonatan-ivanov]

This might be simplified as:

public static Consumer<ChatResponse> setChatResponseInObservationContext(Observation observation) {
		return chatResponse -> {
			if (!observation.isNoop()) {
				((ChatModelObservationContext) observation.getContext()).setResponse(chatResponse)
			}
		};
}

[jxblum]

Same comment as above.

[jonatan-ivanov]

Is this an enum? If so, wouldn't == be ok (/faster)?

[jxblum]

Yes, it is an enum. However, it is always safer to use equals(:Object) than ==. Besides, a properly constructed equals(:Object) method covers == anyway. Let the JIT compiler optimize.

Additionally, while it may not apply in this context, serialization will most certainly invalidate the use of ==.

I'd argue that == should only be used for primitive types, and rarely, if ever Objects.

[jonatan-ivanov]

I guess this cannot be package-private but should we somehow indicate that this is for internal use?
Also, there might be another Observations-related util class iirc.

[jxblum]

Agreed. Documented and moved to the o.s.ai.chat.observation.support package.

[jonatan-ivanov]

ChatModelObservationSupport can be parameterized so it will work for multiple types, This might not be needed.

[jxblum]

I thought about that, too. In the end. I kept Chat and Vector ObservationContext treatment separate for now (i.e. no generics), which are the only 2 use cases I know of (ATM).

[jxblum]

@markpollack

I'm a bit nervous to get this in before Nov 11, but nevertheless do want to know about best practices here - there are many it seems that are a bit opaque to me. How expensive is it to create an ObservationContext? @ThomasVitale @jonatan-ivanov ?

Always your call, of course, but I'd hope that most concerns were covered by tests, which I always run in entirety after each change I make (PR I submit), in addition to adding additional test coverage where needed. :)