Simplify and remove duplicate logic in AI model observation components

[jxblum]

See title.

[ThomasVitale]

The Context is never null as per the Micrometer infrastructure. Is there a reason for making it nullable?

[jxblum]

I am going to remove the @Nullable annotation so it generates the appropriate warnings in the supported IDEs.

[ThomasVitale]

Same thing here, the Context is never null as per the Micrometer infrastructure.

[jxblum]

Same comment as above.

[ThomasVitale]

General comment about the added @Nullable annotations in the observation classes. The Observation.Context passed from the Micrometer infrastructure is never null, it's actually marked as non-null upstream. Is there a reason for making it nullable?

[jxblum]

General comment about the added @Nullable annotations in the observation classes. The Observation.Context passed from the Micrometer infrastructure is never null, it's actually marked as non-null upstream. Is there a reason for making it nullable?

Well, this and this, contradicts the contract.

I would argue that it is never good for any code to throw a NPE, especially in boolean methods that should simply return true or false the majority of the time.

So, I think the supportsContext(:Context) should be "null-safe" and onError(:Context) method have appropriate validation.

In both cases, I have made changes.

[ThomasVitale]

Well, this and this, contradicts the contract.

Thanks for mentioning those two points. I didn't actually notice we were performing a null check there. It might be the only Micrometer API-related implementation where we do that, because all other handlers, predicates, and filters I know of don't do that. I would say those two checks shouldn't be there since the interface is marked with @NonNullApi, establishing the input Observation.Context to be non-null. If we allow null values, we are changing the behaviour of the API.

I would argue that it is never good for any code to throw a NPE, especially in boolean methods that should simply return true or false the majority of the time.

Totally agree with you. But then we should add a check to verify the compliance with the contract, which is a non-null contract, rather than allowing null. And that check is indeed missing. We could add the following. What do you think?

Assert.notNull(context, "context cannot be null);

Micrometer itself ensures that handlers, predicates, and filters are called with a non-null context, but if at some point a null value is passed, then we get an exception thrown since that should never happen and we entered a faulty state of the application.

[ThomasVitale]

Do we need this to be abstract?

[jxblum]

Great question!

For extensibility purposes, I often prefer classes marked as abstract rather than final with a private constructor, even if they mostly, or entirely, contain static methods (e.g. utility classes).

This prevents the class from being immediately instantiated (without extra effort) and allows the class to be extended with another abstract (or concrete) class that may be context-specific. Additionally this helps to reduce the responsibility and footprint of the parent class.

For example, FileSystemUtils to FileUtils to IOUtils (extending nothing).

Of course this is debatable and so I am not a huge stickler on this.

I believe one of Spring's primary strengths is its excellent implementation of the Open/Closed principle. This upholds Spring's principle of choice and gives developers options OOTB if the base functionality does not quite meet their needs.

In fact, in this particular case, I'd probably even prefer that the ChatModelObservationContentProcessor class not contain static methods, and that instance could even be treated as a proper bean.

Again, I am flexible here.

[jxblum]

But then we should add a check to verify the compliance with the contract, which is a non-null contract, rather than allowing null. And that check is indeed missing. We could add the following. What do you think?

Agreed. And, I already changed the PR to do exactly what you suggested.

[jxblum]

Hi @tzolov & @ThomasVitale (CC'ing @jonatan-ivanov),

I did have a question (or 2) about Spring AI's Observability story.

While my questions focus on VectorStore, it is equally applicable to chat as well as other observable bits in Spring AI.

1. I am curious how it was decided what was considered a high-cardinality key vs. low-cardinality key?

For instance, in VectorStoreObservationDocumentation, we see low-cardinality keys and high-cardinality keys defined. I understand and can reason about the low-cardinality keys, although, DB_OPERATION_NAME seems debatable to me, which could be quite "high", depending on the application use cases.

However, some of the high-cardinality keys do not seem as though they would have an "unbounded range of values"; for example: [DB_NAMESPACE, DB_VECTOR_FIELD_NAME, DB_SEARCH_SIMILARITY_METRIC]. Whereas, other keys would definitely have an unbounded range, for example: [DB_VECTOR_QUERY_CONTENT, DB_VECTOR_QUERY_FILTER, DB_VECTOR_QUERY_RESPONSE_DOCUMENTS, DB_VECTOR_QUERY_SIMILARITY_THRESHOLD, DB_VECTOR_QUERY_TOP_K], which naturally vary by request).

I even debated about DB_COLLECTION_NAME, but I can see that the "collection" (e.g. table) could vary quite a lot given a properly organized (or normalized) data store, keeping embeddings for Documents of different kinds separate, thereby reducing table space, among other data management hygiene concerns.

On the other hand, the DB_SEARCH_SIMILARITY_METRIC is definitely bounded, presently to a pre-defined enumeration.

2. This is probably more of a Micrometer question, but does the KeyNames and KeyValues need to match for each of the high-cardinality and low-cardinality keys?

This seems logical to me that they nicely align in this way.

NOTE: I only called out the high-cardinality keys and values in my references above for #2, by way of example.