Address review comments

spring-projects · Aug 11, 2024 · 15e06cd · 15e06cd
1 parent 4489902
commit 15e06cd
Show file tree

Hide file tree

Showing 9 changed files with 33 additions and 64 deletions.
diff --git a/spring-ai-core/src/main/java/org/springframework/ai/chat/client/DefaultChatClient.java b/spring-ai-core/src/main/java/org/springframework/ai/chat/client/DefaultChatClient.java
@@ -38,7 +38,6 @@
 import org.springframework.ai.chat.model.ChatModel;
 import org.springframework.ai.chat.model.ChatResponse;
 import org.springframework.ai.chat.model.StreamingChatModel;
-import org.springframework.ai.chat.observation.ChatModelObservationDocumentation;
 import org.springframework.ai.chat.prompt.ChatOptions;
 import org.springframework.ai.chat.prompt.Prompt;
 import org.springframework.ai.chat.prompt.PromptTemplate;
@@ -337,7 +336,6 @@ private ChatResponse doGetObservableChatResponse(DefaultChatClientRequestSpec in
 						() -> observationContext, inputRequest.observationRegistry)
 				.observe(() -> {
 					ChatResponse chatResponse = doGetChatResponse(inputRequest, formatParam);
-					observationContext.setChatResponse(chatResponse);
 					return chatResponse;
 				});
 
@@ -440,7 +438,6 @@ private Flux<ChatResponse> doGetFluxChatResponse(DefaultChatClientRequestSpec in
 
 				// @formatter:off
 				return doGetFluxChatResponse2(inputRequest)
-					.doOnNext(cr -> observationContext.setChatResponse(cr))
 					.doOnError(observation::error)
 					.doFinally(s -> {
 						observation.stop();

diff --git a/...g/springframework/ai/chat/client/observation/ChatClientInputContentObservationFilter.java b/...g/springframework/ai/chat/client/observation/ChatClientInputContentObservationFilter.java
@@ -42,7 +42,7 @@ public class ChatClientInputContentObservationFilter implements ObservationFilte
 		.of(ChatClientObservationDocumentation.HighCardinalityKeyNames.CHAT_CLIENT_USER_TEXT, KeyValue.NONE_VALUE);
 
 	private static final KeyValue CHAT_CLIENT_USER_PARAM_NONE = KeyValue
-		.of(ChatClientObservationDocumentation.HighCardinalityKeyNames.CHAT_CLIENT_USER_PARAM, KeyValue.NONE_VALUE);
+		.of(ChatClientObservationDocumentation.HighCardinalityKeyNames.CHAT_CLIENT_USER_PARAMS, KeyValue.NONE_VALUE);
 
 	@Override
 	public Observation.Context map(Observation.Context context) {
@@ -91,7 +91,7 @@ protected KeyValue chatClientUserParam(ChatClientObservationContext context) {
 		if (CollectionUtils.isEmpty(context.getRequest().getUserParams())) {
 			return CHAT_CLIENT_USER_PARAM_NONE;
 		}
-		return KeyValue.of(ChatClientObservationDocumentation.HighCardinalityKeyNames.CHAT_CLIENT_USER_PARAM,
+		return KeyValue.of(ChatClientObservationDocumentation.HighCardinalityKeyNames.CHAT_CLIENT_USER_PARAMS,
 				context.getRequest()
 					.getUserParams()
 					.entrySet()

diff --git a/...ain/java/org/springframework/ai/chat/client/observation/ChatClientObservationContext.java b/...ain/java/org/springframework/ai/chat/client/observation/ChatClientObservationContext.java
@@ -16,37 +16,40 @@
 package org.springframework.ai.chat.client.observation;
 
 import org.springframework.ai.chat.client.DefaultChatClient.DefaultChatClientRequestSpec;
-import org.springframework.ai.chat.model.ChatResponse;
-import org.springframework.ai.model.observation.ModelObservationContext;
 import org.springframework.ai.observation.AiOperationMetadata;
 
+import io.micrometer.observation.Observation;
+
 /**
  * @author Christian Tzolov
  * @since 1.0.0
  */
 
-public class ChatClientObservationContext extends ModelObservationContext<DefaultChatClientRequestSpec, ChatResponse> {
-
-	private ChatResponse chatResponse;
+public class ChatClientObservationContext extends Observation.Context {
 
 	private final boolean stream;
 
 	private final String format;
 
+	private final DefaultChatClientRequestSpec request;
+
+	private final AiOperationMetadata operationMetadata;
+
 	public ChatClientObservationContext(DefaultChatClientRequestSpec requestSpec, AiOperationMetadata operationMetadata,
 			String format, Boolean isStream) {
 
-		super(requestSpec, operationMetadata);
+		this.request = requestSpec;
+		this.operationMetadata = operationMetadata;
 		this.format = format;
 		this.stream = isStream;
 	}
 
-	public ChatResponse getChatResponse() {
-		return this.chatResponse;
+	public DefaultChatClientRequestSpec getRequest() {
+		return this.request;
 	}
 
-	public void setChatResponse(ChatResponse chatResponse) {
-		this.chatResponse = chatResponse;
+	public AiOperationMetadata getOperationMetadata() {
+		return this.operationMetadata;
 	}
 
 	public boolean isStream() {

diff --git a/...va/org/springframework/ai/chat/client/observation/ChatClientObservationDocumentation.java b/...va/org/springframework/ai/chat/client/observation/ChatClientObservationDocumentation.java
@@ -42,7 +42,7 @@ public KeyName[] getLowCardinalityKeyNames() {
 
 		@Override
 		public KeyName[] getHighCardinalityKeyNames() {
-			return new KeyName[] {};
+			return HighCardinalityKeyNames.values();
 		}
 
 	};
@@ -67,17 +67,6 @@ public String asString() {
 			public String asString() {
 				return "chat.client.stream";
 			}
-		},
-
-		/**
-		 * ChatModel response raw status code, or {@code "IO_ERROR"} in case of
-		 * {@code IOException}, or {@code "CLIENT_ERROR"} if no response was received.
-		 */
-		STATUS {
-			@Override
-			public String asString() {
-				return "chat.client.status";
-			}
 		}
 
 	}
@@ -106,7 +95,7 @@ public String asString() {
 		/**
 		 * List of configured chat client advisors.
 		 */
-		CHAT_CLIENT_ADVISOR {
+		CHAT_CLIENT_ADVISORS {
 			@Override
 			public String asString() {
 				return "spring.ai.chat.client.advisors";
@@ -115,7 +104,7 @@ public String asString() {
 		/**
 		 * Map of advisor parameters.
 		 */
-		CHAT_CLIENT_ADVISOR_PARAM {
+		CHAT_CLIENT_ADVISOR_PARAMS {
 			@Override
 			public String asString() {
 				return "spring.ai.chat.client.advisor.params";
@@ -133,7 +122,7 @@ public String asString() {
 		/**
 		 * Chat client user parameters.
 		 */
-		CHAT_CLIENT_USER_PARAM {
+		CHAT_CLIENT_USER_PARAMS {
 			@Override
 			public String asString() {
 				return "spring.ai.chat.client.user.params";

diff --git a/...rg/springframework/ai/chat/client/observation/DefaultChatClientObservationConvention.java b/...rg/springframework/ai/chat/client/observation/DefaultChatClientObservationConvention.java
@@ -34,8 +34,6 @@ public class DefaultChatClientObservationConvention implements ChatClientObserva
 
 	public static final String DEFAULT_NAME = "gen_ai.chat.client.operation";
 
-	private static final KeyValue STATUS_NONE = KeyValue.of(LowCardinalityKeyNames.STATUS, KeyValue.NONE_VALUE);
-
 	private static final KeyValue CHAT_CLIENT_TOOL_FUNCTION_CALLBACKS_NONE = KeyValue.of(
 			ChatClientObservationDocumentation.HighCardinalityKeyNames.CHAT_CLIENT_TOOL_FUNCTION_CALLBACKS,
 			KeyValue.NONE_VALUE);
@@ -45,10 +43,10 @@ public class DefaultChatClientObservationConvention implements ChatClientObserva
 			KeyValue.NONE_VALUE);
 
 	private static final KeyValue CHAT_CLIENT_ADVISOR_NONE = KeyValue
-		.of(ChatClientObservationDocumentation.HighCardinalityKeyNames.CHAT_CLIENT_ADVISOR, KeyValue.NONE_VALUE);
+		.of(ChatClientObservationDocumentation.HighCardinalityKeyNames.CHAT_CLIENT_ADVISORS, KeyValue.NONE_VALUE);
 
 	private static final KeyValue CHAT_CLIENT_ADVISOR_PARAM_NONE = KeyValue
-		.of(ChatClientObservationDocumentation.HighCardinalityKeyNames.CHAT_CLIENT_ADVISOR_PARAM, KeyValue.NONE_VALUE);
+		.of(ChatClientObservationDocumentation.HighCardinalityKeyNames.CHAT_CLIENT_ADVISOR_PARAMS, KeyValue.NONE_VALUE);
 
 	private final String name;
 
@@ -74,8 +72,7 @@ public String getContextualName(ChatClientObservationContext context) {
 
 	@Override
 	public KeyValues getLowCardinalityKeyValues(ChatClientObservationContext context) {
-		return KeyValues.of(springAiKind(context), aiOperationType(context), aiProvider(context), stream(context),
-				status(context));
+		return KeyValues.of(springAiKind(context), aiOperationType(context), aiProvider(context), stream(context));
 	}
 
 	@Override
@@ -92,10 +89,6 @@ protected KeyValue stream(ChatClientObservationContext context) {
 		return KeyValue.of(LowCardinalityKeyNames.STREAM, "" + context.isStream());
 	}
 
-	protected KeyValue status(ChatClientObservationContext context) {
-		return STATUS_NONE;
-	}
-
 	protected KeyValue aiOperationType(ChatClientObservationContext context) {
 		return KeyValue.of(ChatModelObservationDocumentation.LowCardinalityKeyNames.AI_OPERATION_TYPE,
 				context.getOperationMetadata().operationType());
@@ -131,15 +124,15 @@ protected KeyValue chatClientAvisor(ChatClientObservationContext context) {
 		if (CollectionUtils.isEmpty(context.getRequest().getAdvisors())) {
 			return CHAT_CLIENT_ADVISOR_NONE;
 		}
-		return KeyValue.of(ChatClientObservationDocumentation.HighCardinalityKeyNames.CHAT_CLIENT_ADVISOR,
+		return KeyValue.of(ChatClientObservationDocumentation.HighCardinalityKeyNames.CHAT_CLIENT_ADVISORS,
 				context.getRequest().getAdvisors().stream().map(a -> a.getName()).collect(Collectors.joining(",")));
 	}
 
 	protected KeyValue chatClientAvisorParam(ChatClientObservationContext context) {
 		if (CollectionUtils.isEmpty(context.getRequest().getAdvisorParams())) {
 			return CHAT_CLIENT_ADVISOR_PARAM_NONE;
 		}
-		return KeyValue.of(ChatClientObservationDocumentation.HighCardinalityKeyNames.CHAT_CLIENT_ADVISOR_PARAM,
+		return KeyValue.of(ChatClientObservationDocumentation.HighCardinalityKeyNames.CHAT_CLIENT_ADVISOR_PARAMS,
 				context.getRequest()
 					.getAdvisorParams()
 					.entrySet()

diff --git a/...ingframework/ai/chat/client/observation/ChatClientInputContentObservationFilterTests.java b/...ingframework/ai/chat/client/observation/ChatClientInputContentObservationFilterTests.java
@@ -89,7 +89,7 @@ void whenWithTextThenAugmentContext() {
 		assertThat(augmentedContext.getHighCardinalityKeyValues())
 			.contains(KeyValue.of(HighCardinalityKeyNames.CHAT_CLIENT_USER_TEXT.asString(), "sample user text"));
 		assertThat(augmentedContext.getHighCardinalityKeyValues())
-			.contains(KeyValue.of(HighCardinalityKeyNames.CHAT_CLIENT_USER_PARAM.asString(), "up1:upv1"));
+			.contains(KeyValue.of(HighCardinalityKeyNames.CHAT_CLIENT_USER_PARAMS.asString(), "up1:upv1"));
 		assertThat(augmentedContext.getHighCardinalityKeyValues())
 			.contains(KeyValue.of(HighCardinalityKeyNames.CHAT_CLIENT_SYSTEM_TEXT.asString(), "sample system text"));
 		assertThat(augmentedContext.getHighCardinalityKeyValues())

diff --git a/...ringframework/ai/chat/client/observation/DefaultChatClientObservationConventionTests.java b/...ringframework/ai/chat/client/observation/DefaultChatClientObservationConventionTests.java
@@ -142,8 +142,8 @@ void shouldHaveOptionalKeyValues() {
 				generateOperationMetadata(), "json", true);
 
 		assertThat(this.observationConvention.getHighCardinalityKeyValues(observationContext)).contains(
-				KeyValue.of(HighCardinalityKeyNames.CHAT_CLIENT_ADVISOR.asString(), "advisor1,advisor2"),
-				KeyValue.of(HighCardinalityKeyNames.CHAT_CLIENT_ADVISOR_PARAM.asString(),
+				KeyValue.of(HighCardinalityKeyNames.CHAT_CLIENT_ADVISORS.asString(), "advisor1,advisor2"),
+				KeyValue.of(HighCardinalityKeyNames.CHAT_CLIENT_ADVISOR_PARAMS.asString(),
 						"advParam1:advisorParam1Value"),
 				KeyValue.of(HighCardinalityKeyNames.CHAT_CLIENT_TOOL_FUNCTION_NAMES.asString(), "function1,function2"),
 				KeyValue.of(HighCardinalityKeyNames.CHAT_CLIENT_TOOL_FUNCTION_CALLBACKS.asString(),

diff --git a/...in/java/org/springframework/ai/autoconfigure/chat/client/ChatClientAutoConfiguration.java b/...in/java/org/springframework/ai/autoconfigure/chat/client/ChatClientAutoConfiguration.java
@@ -79,7 +79,7 @@ ChatClient.Builder chatClientBuilder(ChatClientBuilderConfigurer chatClientBuild
 
 	@Bean
 	@ConditionalOnMissingBean
-	@ConditionalOnProperty(prefix = ChatClientBuilderProperties.CONFIG_PREFIX + ".observation", name = "include-input",
+	@ConditionalOnProperty(prefix = ChatClientBuilderProperties.CONFIG_PREFIX + ".observations", name = "include-input",
 			havingValue = "true")
 	ChatClientInputContentObservationFilter chatClientInputContentObservationFilter() {
 		logger.warn(

diff --git a/...in/java/org/springframework/ai/autoconfigure/chat/client/ChatClientBuilderProperties.java b/...in/java/org/springframework/ai/autoconfigure/chat/client/ChatClientBuilderProperties.java
@@ -24,7 +24,7 @@
  * @author Mark Pollack
  * @author Josh Long
  * @author Arjen Poutsma
- * @since 1.0.0 M1
+ * @since 1.0.0
  */
 @ConfigurationProperties(ChatClientBuilderProperties.CONFIG_PREFIX)
 public class ChatClientBuilderProperties {
@@ -36,10 +36,10 @@ public class ChatClientBuilderProperties {
 	 */
 	private boolean enabled = true;
 
-	private Observation observation = new Observation();
+	private Observations observations = new Observations();
 
-	public Observation getObservation() {
-		return this.observation;
+	public Observations getObservations() {
+		return this.observations;
 	}
 
 	public boolean isEnabled() {
@@ -50,18 +50,13 @@ public void setEnabled(boolean enabled) {
 		this.enabled = enabled;
 	}
 
-	public static class Observation {
+	public static class Observations {
 
 		/**
 		 * Whether to include the input content in the observations.
 		 */
 		private boolean includeInput = false;
 
-		/**
-		 * Whether to include the output content in the observations.
-		 */
-		private boolean includeOutput = false;
-
 		public boolean isIncludeInput() {
 			return includeInput;
 		}
@@ -70,14 +65,6 @@ public void setIncludeInput(boolean includeCompletion) {
 			this.includeInput = includeCompletion;
 		}
 
-		public boolean isIncludeOutput() {
-			return includeOutput;
-		}
-
-		public void setIncludeOutput(boolean includePrompt) {
-			this.includeOutput = includePrompt;
-		}
-
 	}
 
 }