Skip to content

Simplify and remove duplicate logic in AI model observation components #1653

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Simplify and remove duplicate logic in AI model observation components #1653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions ...in/java/org/springframework/ai/chat/observation/ChatModelCompletionObservationFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,26 +22,28 @@
import org.springframework.ai.observation.tracing.TracingHelper;

/**
* An {@link ObservationFilter} to include the chat completion content in the observation.
* {@link ObservationFilter} used to include chat completion content in the
* {@link Observation}.
*
* @author Thomas Vitale
* @author John Blum
* @since 1.0.0
*/
public class ChatModelCompletionObservationFilter implements ObservationFilter {

@Override
public Observation.Context map(Observation.Context context) {
if (!(context instanceof ChatModelObservationContext chatModelObservationContext)) {
return context;
}

var completions = ChatModelObservationContentProcessor.completion(chatModelObservationContext);
if (context instanceof ChatModelObservationContext chatModelObservationContext) {

var completions = ChatModelObservationContentProcessor.completion(chatModelObservationContext);

chatModelObservationContext
.addHighCardinalityKeyValue(ChatModelObservationDocumentation.HighCardinalityKeyNames.COMPLETION
.withValue(TracingHelper.concatenateStrings(completions)));
context = chatModelObservationContext
.addHighCardinalityKeyValue(ChatModelObservationDocumentation.HighCardinalityKeyNames.COMPLETION
.withValue(TracingHelper.concatenateStrings(completions)));
}

return chatModelObservationContext;
return context;
}

}
26 changes: 20 additions & 6 deletions ...c/main/java/org/springframework/ai/chat/observation/ChatModelMeterObservationHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,25 @@

package org.springframework.ai.chat.observation;

import java.util.Optional;

import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.observation.Observation;
import io.micrometer.observation.ObservationHandler;

import org.springframework.ai.chat.metadata.ChatResponseMetadata;
import org.springframework.ai.chat.metadata.Usage;
import org.springframework.ai.chat.model.ChatResponse;
import org.springframework.ai.model.observation.ModelUsageMetricsGenerator;
import org.springframework.lang.Nullable;

/**
* Handler for generating metrics from chat model observations.
* {@link ObservationHandler} used to generate metrics from chat model observations.
*
* @author Thomas Vitale
* @author John Blum
* @see ChatModelObservationContext
* @see ObservationHandler
* @since 1.0.0
*/
public class ChatModelMeterObservationHandler implements ObservationHandler<ChatModelObservationContext> {
Expand All @@ -38,11 +47,16 @@ public ChatModelMeterObservationHandler(MeterRegistry meterRegistry) {

@Override
public void onStop(ChatModelObservationContext context) {
if (context.getResponse() != null && context.getResponse().getMetadata() != null
&& context.getResponse().getMetadata().getUsage() != null) {
ModelUsageMetricsGenerator.generate(context.getResponse().getMetadata().getUsage(), context,
this.meterRegistry);
}
resolveUsage(context)
.ifPresent(usage -> ModelUsageMetricsGenerator.generate(usage, context, this.meterRegistry));
}

private Optional<Usage> resolveUsage(@Nullable ChatModelObservationContext context) {

return Optional.ofNullable(context)
.map(ChatModelObservationContext::getResponse)
.map(ChatResponse::getMetadata)
.map(ChatResponseMetadata::getUsage);
}

@Override
Expand Down
43 changes: 21 additions & 22 deletions ...in/java/org/springframework/ai/chat/observation/ChatModelObservationContentProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,46 +16,45 @@

package org.springframework.ai.chat.observation;

import java.util.Collections;
import java.util.List;
import java.util.Optional;

import org.springframework.ai.chat.messages.Message;
import org.springframework.ai.chat.model.ChatResponse;
import org.springframework.ai.chat.model.Generation;
import org.springframework.ai.model.Content;
import org.springframework.lang.Nullable;
import org.springframework.util.CollectionUtils;
import org.springframework.util.StringUtils;

/**
* Utilities to process the prompt and completion content in observations for chat models.
*
* @author Thomas Vitale
* @author John Blum
* @since 1.0.0
*/
public final class ChatModelObservationContentProcessor {

private ChatModelObservationContentProcessor() {
}
public abstract class ChatModelObservationContentProcessor {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this to be abstract?

Copy link
Contributor Author

@jxblum jxblum Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question!

For extensibility purposes, I often prefer classes marked as abstract rather than final with a private constructor, even if they mostly, or entirely, contain static methods (e.g. utility classes).

This prevents the class from being immediately instantiated (without extra effort) and allows the class to be extended with another abstract (or concrete) class that may be context-specific. Additionally this helps to reduce the responsibility and footprint of the parent class.

For example, FileSystemUtils to FileUtils to IOUtils (extending nothing).

Of course this is debatable and so I am not a huge stickler on this.

I believe one of Spring's primary strengths is its excellent implementation of the Open/Closed principle. This upholds Spring's principle of choice and gives developers options OOTB if the base functionality does not quite meet their needs.

In fact, in this particular case, I'd probably even prefer that the ChatModelObservationContentProcessor class not contain static methods, and that instances could even be treated as proper beans.

Again, I am flexible here.


public static List<String> prompt(ChatModelObservationContext context) {
if (CollectionUtils.isEmpty(context.getRequest().getInstructions())) {
return List.of();
}

return context.getRequest().getInstructions().stream().map(Content::getContent).toList();
}
List<Message> instructions = context.getRequest().getInstructions();

public static List<String> completion(ChatModelObservationContext context) {
if (context == null || context.getResponse() == null || context.getResponse().getResults() == null
|| CollectionUtils.isEmpty(context.getResponse().getResults())) {
return List.of();
}
return CollectionUtils.isEmpty(instructions) ? Collections.emptyList()
: instructions.stream().map(Content::getContent).toList();
}

if (!StringUtils.hasText(context.getResponse().getResult().getOutput().getContent())) {
return List.of();
}
public static List<String> completion(@Nullable ChatModelObservationContext context) {

return context.getResponse()
.getResults()
return Optional.ofNullable(context)
.map(ChatModelObservationContext::getResponse)
.map(ChatResponse::getResults)
.orElseGet(Collections::emptyList)
.stream()
.filter(generation -> generation.getOutput() != null
&& StringUtils.hasText(generation.getOutput().getContent()))
.map(generation -> generation.getOutput().getContent())
.map(Generation::getOutput)
.map(Message::getContent)
.filter(StringUtils::hasText)
.toList();
}

Expand Down
17 changes: 9 additions & 8 deletions ...java/org/springframework/ai/chat/observation/ChatModelPromptContentObservationFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,23 +25,24 @@
* An {@link ObservationFilter} to include the chat prompt content in the observation.
*
* @author Thomas Vitale
* @author John Blum
* @since 1.0.0
*/
public class ChatModelPromptContentObservationFilter implements ObservationFilter {

@Override
public Observation.Context map(Observation.Context context) {
if (!(context instanceof ChatModelObservationContext chatModelObservationContext)) {
return context;
}

var prompts = ChatModelObservationContentProcessor.prompt(chatModelObservationContext);
if (context instanceof ChatModelObservationContext chatModelObservationContext) {

var prompts = ChatModelObservationContentProcessor.prompt(chatModelObservationContext);

chatModelObservationContext
.addHighCardinalityKeyValue(ChatModelObservationDocumentation.HighCardinalityKeyNames.PROMPT
.withValue(TracingHelper.concatenateStrings(prompts)));
context = chatModelObservationContext
.addHighCardinalityKeyValue(ChatModelObservationDocumentation.HighCardinalityKeyNames.PROMPT
.withValue(TracingHelper.concatenateStrings(prompts)));
}

return chatModelObservationContext;
return context;
}

}
Loading