Improve chat client and advisor observations #1472
Conversation
ThomasVitale
commented
Oct 6, 2024
ThomasVitale
commented
- Make ChatClient and Advisor observation logic null-safe
- Simplify naming for Advisor observations
- Include high-cardinality attributes only if a value is present
- Fix condition to include system test to chat client observations
- Add Advisor order information to context
- Streamline usage of enums and utils to reduce hard-coded/duplications
- Fix pending Chroma integration test
* Make ChatClient and Advisor observation logic null-safe * Simplify naming for Advisor observations * Include high-cardinality attributes only if a value is present * Fix condition to include system test to chat client observations * Add Advisor order information to context * Streamline usage of enums and utils to reduce hard-coded/duplications * Fix pending Chroma integration test Signed-off-by: Thomas Vitale <[email protected]>
| .withAdvisorType(AdvisorObservationContext.Type.AROUND) | ||
| .withAdvisedRequest(advisedRequest) | ||
| .withAdvisorRequestContext(advisedRequest.adviseContext()) | ||
| .withOrder(advisor.getOrder()) |
There was a problem hiding this comment.
I noticed we were not passing the "order" field to the observation context, so I added it both for imperative and stream operations
There was a problem hiding this comment.
I've added the order to the context but apparently missed to populate it
| @Override | ||
| public String asString() { | ||
| return "spring.ai.chat.client.advisor.type"; | ||
| return "spring.ai.advisor.type"; |
There was a problem hiding this comment.
@tzolov what do you think about this simplification of the naming for Advisor attributes?
There was a problem hiding this comment.
The advisors are only applicable in the context of ChatClient.
But i guess it is enough to reflect this int the observation's name (e.g. spring.ai.chat.client.advisor) and not in the attribute names?
There was a problem hiding this comment.
The related span is also nested under a "chat_client" parent span, so that might be enough. But if we should have it in the attribute name as well, I can revert this
| if (!StringUtils.hasText(context.getRequest().getUserText())) { | ||
| return CHAT_CLIENT_SYSTEM_TEXT_NONE; | ||
| protected void chatClientSystemText(ChatClientObservationContext context) { | ||
| if (!StringUtils.hasText(context.getRequest().getSystemText())) { |
There was a problem hiding this comment.
Here the condition was checking the user text instead of the system text, I fixed it now
| @Override | ||
| public String asString() { | ||
| return "spring.ai.chat.client.tool.functioncallbacks"; | ||
| return "spring.ai.chat.client.tool.function.callbacks"; |
There was a problem hiding this comment.
@tzolov I made this change to align with the other attributes called "*.function.names". Is this ok?
|
|
||
| private static final KeyValue ADVISOR_NAME_NONE = KeyValue.of(HighCardinalityKeyNames.ADVISOR_NAME, | ||
| KeyValue.NONE_VALUE); | ||
| public static final String DEFAULT_NAME = "spring.ai.advisor"; |
There was a problem hiding this comment.
@tzolov this name is only used for metrics. Since Micrometer generates several timer-related metrics with long names, I thought of making this base name shorter as well.
So, instead of having metrics with name "spring_ai_chat_client_advisor_active_milliseconds_count" we would have "spring_ai_advisor_active_milliseconds_count".
Thoughts? Since Advisors are not used anywhere else, we might not need the "chat.client" part in the base name.
|
LGTM, thanks @ThomasVitale |
2 participants
