See title.

Comment From: 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?

Comment From: 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.

Comment From: 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.

Comment From: 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.

Comment From: 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 can 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.