feat: Make advisors observations configurable #4350
Conversation
ThomasVitale
commented
Sep 9, 2025
ThomasVitale
commented
- Add possibility to provide a custom AdvisorObservationConvention to the ChatClient for customizing the conventions for advisor spans and metrics.
- Add ChatClientResponse to ChatClientObservationContext for achiving full visibility into both request and response.
* Add possibility to provide a custom AdvisorObservationConvention to the ChatClient for customizing the conventions for advisor spans and metrics. * Add ChatClientResponse to ChatClientObservationContext for achiving full visibility into both request and response. Signed-off-by: Thomas Vitale <[email protected]>
ThomasVitale
force-pushed
the
gh-missing-observations-chat-client
branch
from
e53bac4 to
0f61161
Compare
|
@jonatan-ivanov could you review this? |
spring-ai-client-chat/src/main/java/org/springframework/ai/chat/client/ChatClient.java
Outdated
Show resolved
Hide resolved
| @Nullable | ||
| public ChatClientResponse getChatClientResponse() { | ||
| return this.chatClientResponse; | ||
| } | ||
|
|
||
| public void setChatClientResponse(@Nullable ChatClientResponse chatClientResponse) { | ||
| this.chatClientResponse = chatClientResponse; | ||
| } | ||
|
|
There was a problem hiding this comment.
This will not work in the streaming (Reactor) scenario.
Maybe we should fix that first in a separate PR (along with the logging use-case), I have a PoC for that: main...jonatan-ivanov:spring-ai:promt-on-span
There was a problem hiding this comment.
Both the ChatClient and Advisor observation contexts don't get the response populated at the moment, for the reactive scenarios.
I didn't to that here since I don't feel comfortable making changes via the reactor context propagation.
So it would be great if it could be added in a separate PR, thank you!
There was a problem hiding this comment.
The ChatClientResponse was already part of the AdvisorObservationContext, but it was never populated. In this PR, I made a change to populate it in non-streaming scenarios.
And for consistency, I've done the same for ChatClientObservationContext.
There was a problem hiding this comment.
What do you think about this? #4417
The interesting part is small, see: DefaultChatClient and ChatClientObservationContext. This PR could depend on this and we can also re-use this in the prompt/completion on spans scenario.
There was a problem hiding this comment.
Thanks for sharing! I checked the PR and looks good to me. The only comment I left is about the ChatClientObservationContext: #4417 (comment)
There was a problem hiding this comment.
If you agree with that comment, I would suggest having that PR build on this one, adding support for the reactive scenario and completion logs.
Signed-off-by: Thomas Vitale <[email protected]>
4 participants
