-
Notifications
You must be signed in to change notification settings - Fork 2k
Improve chat client and advisor observations #1472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,7 @@ | |
| public enum AdvisorObservationDocumentation implements ObservationDocumentation { | ||
|
|
||
| /** | ||
| * AI Chat Client observations | ||
| * AI Advisor observations | ||
| */ | ||
| AI_ADVISOR { | ||
| @Override | ||
|
|
@@ -65,7 +65,7 @@ public String asString() { | |
| ADVISOR_TYPE { | ||
| @Override | ||
| public String asString() { | ||
| return "spring.ai.chat.client.advisor.type"; | ||
| return "spring.ai.advisor.type"; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tzolov what do you think about this simplification of the naming for Advisor attributes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The advisors are only applicable in the context of ChatClient. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -74,12 +74,12 @@ public String asString() { | |
| public enum HighCardinalityKeyNames implements KeyName { | ||
|
|
||
| /** | ||
| * Chat Model name. | ||
| * Advisor name. | ||
| */ | ||
| ADVISOR_NAME { | ||
| @Override | ||
| public String asString() { | ||
| return "spring.ai.chat.client.advisor.name"; | ||
| return "spring.ai.advisor.name"; | ||
| } | ||
| }, | ||
| /** | ||
|
|
@@ -88,7 +88,7 @@ public String asString() { | |
| ADVISOR_ORDER { | ||
| @Override | ||
| public String asString() { | ||
| return "spring.ai.chat.client.advisor.order"; | ||
| return "spring.ai.advisor.order"; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
|
|
||
| import org.springframework.ai.chat.client.advisor.observation.AdvisorObservationDocumentation.HighCardinalityKeyNames; | ||
| import org.springframework.ai.chat.client.advisor.observation.AdvisorObservationDocumentation.LowCardinalityKeyNames; | ||
| import org.springframework.ai.observation.conventions.SpringAiKind; | ||
| import org.springframework.ai.util.ParsingUtils; | ||
| import org.springframework.lang.Nullable; | ||
|
|
||
|
|
@@ -27,18 +28,9 @@ | |
| * @author Christian Tzolov | ||
| * @since 1.0.0 | ||
| */ | ||
|
|
||
| public class DefaultAdvisorObservationConvention implements AdvisorObservationConvention { | ||
|
|
||
| public static final String DEFAULT_NAME = "spring.ai.chat.client.advisor"; | ||
|
|
||
| private static final String CHAT_CLIENT_ADVISOR_SPRING_AI_KIND = "chat_client_advisor"; | ||
|
|
||
| private static final KeyValue ADVISOR_TYPE_NONE = KeyValue.of(LowCardinalityKeyNames.ADVISOR_TYPE, | ||
| KeyValue.NONE_VALUE); | ||
|
|
||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||
|
|
||
| private final String name; | ||
|
|
||
|
|
@@ -73,15 +65,12 @@ public KeyValues getLowCardinalityKeyValues(AdvisorObservationContext context) { | |
| } | ||
|
|
||
| protected KeyValue advisorType(AdvisorObservationContext context) { | ||
| if (context.getAdvisorType() != null) { | ||
| return KeyValue.of(LowCardinalityKeyNames.ADVISOR_TYPE, context.getAdvisorType().name()); | ||
| } | ||
| return ADVISOR_TYPE_NONE; | ||
| return KeyValue.of(LowCardinalityKeyNames.ADVISOR_TYPE, context.getAdvisorType().name()); | ||
| } | ||
|
|
||
| protected KeyValue springAiKind() { | ||
| return KeyValue.of(AdvisorObservationDocumentation.LowCardinalityKeyNames.SPRING_AI_KIND, | ||
| CHAT_CLIENT_ADVISOR_SPRING_AI_KIND); | ||
| SpringAiKind.ADVISOR.value()); | ||
| } | ||
|
|
||
| // ------------------------ | ||
|
|
@@ -94,14 +83,11 @@ public KeyValues getHighCardinalityKeyValues(AdvisorObservationContext context) | |
| } | ||
|
|
||
| protected KeyValue advisorName(AdvisorObservationContext context) { | ||
| if (context.getAdvisorType() != null) { | ||
| return KeyValue.of(HighCardinalityKeyNames.ADVISOR_NAME, context.getAdvisorName()); | ||
| } | ||
| return ADVISOR_NAME_NONE; | ||
| return KeyValue.of(HighCardinalityKeyNames.ADVISOR_NAME, context.getAdvisorName()); | ||
| } | ||
|
|
||
| protected KeyValue advisorOrder(AdvisorObservationContext context) { | ||
| return KeyValue.of(HighCardinalityKeyNames.ADVISOR_ORDER, "" + context.getOrder()); | ||
| } | ||
|
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| /* | ||
| * Copyright 2024 the original author or authors. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| @NonNullApi | ||
| @NonNullFields | ||
| package org.springframework.ai.chat.client.advisor.observation; | ||
|
|
||
| import org.springframework.lang.NonNullApi; | ||
| import org.springframework.lang.NonNullFields; |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the order to the context but apparently missed to populate it