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.

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)

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());
}

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.

You can do this instead (might be simpler):

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

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);
  }

  // ...
}

This might be simplified as:

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

Same comment as above.

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

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.

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.

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

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

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).

Is this valid?
Should not it be one } less at the end?

If so, can we fix it in this PR?

Hi @jonatan-ivanov - I am not following your question here.

In source, currently (line numbers changed a bit now since I last submitted this PR), there is already 1 too many } in the logger statement.

The change I made removes the extra (unnecessary) } at the end.

I'm talking about a different line: https://github.com/spring-projects/spring-ai/pull/1661/files#diff-4b360f06ee830dd00855e2fd3e0d00b78a3aa23ca4257145e71cd2d03a7f1caeR244

Gosh, it has been so long since I looked at this PR, I had to open my IDE to see the precise changes I made since IJ does a better job at highlighting the "actual" changes vs. what appears in GitHub.

Even in my PR, though it would appear with Git blame that I changed the lines (which is technically due to formatting), I did not introduce the extra } in the logger statement. In fact, I did not even change either logging statement (2) in this method.

Once again, if you look at the actual, "current" source line compared to the same line in my topic branch for the PR (with no Git blame), they are precisely the same.

Apparently, I did not change either logger statement at all.

What did change is:

1. I introduced a observation local variable (here) to make this less busy.

2. Additionally, I introduced a space after the choices List to be consistent with the separation between the varCompletion and subsequent if block, above. See original source have no space.

This logger statement should be fixed though.

FTR, and for additional clarity, this is what I see in IJ (on my topic branch pr/observation for this PR):

So can we fix line 244 (on the screenshot) in this PR too?

Doing the null check inside and outside of this method depending on the field threw me over a bit, would not it be better to use choice as the parameter?

I don't understand your comment.

Bear with me since line numbers in the source (now) have significantly changed since the PR, but all I was doing here was...

Simplify the Map construction nested in the Lambda (original source), and refactoring into a method, thereby simplifying and making the Lambda more readable.

What puzzled me that we are doing null checks and default setting inside of choiceMetadata for certain values AND we are also doing null checks and default setting here when we call choiceMetadata (outside of it). As a user of choiceMetadata, it's not trivial what parameters should I null-check-and-set-default and what I should not since choiceMetadata will take care about them.

Can we suppress just what is necessary?

That would be nice. Unfortunately, Java falls short on this one. :(

I often wondered the same thing, and of course, there is no shortage of advice on the topic; for example - see first answer.

This is the compiler warning (null check) visible in IJ (not sure about Eclipse):

As you can see, using the "suggestion" from SO of @SuppressWarnings("null"), the "null check" compiler warning is still present leaving the code highlighted in the IDE.

At last, when we review the JLS for Java 17 on @SuppressWarnings annotation, there is no "null" value for @SuppressWarnings and the reason the compiler warning persists.

For good measure, I even tried @SuppressWarnings("unchecked"). That creates a "Redundant expression" issue IJ. Gah! o.O

This...

So both IJ and javac honor @SuppressWarnings("all") for better or worse.