-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(observability): Refactor content observation mechanism to support both logging and tracing #3612
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
base: main
Are you sure you want to change the base?
Conversation
b212a6f
to
c485721
Compare
…t both logging and tracing This commit refactors the content observation mechanism by adding support for trace recording without altering the original logging functionality. The motivation for this change is to ensure complete context availability in scenarios such as integration with Langfuse, which is essential for proper functionality. Main changes: 1. New configuration options: 1. `trace-prompt`: Enables recording prompts to trace 2. `trace-completion` : Enables recording completions to trace 3. `trace-prompt-size` : Limits the length of prompt context 4. `content-formatter` : Handles content formatting, currently supporting both 'text' and 'langfuse' modes 2. Added two handlers, ChatModelCompletionObservationTraceHandler and ChatModelPromptContentObservationTraceHandler, to support recording context to trace. 3. Introduced the MessageFormatter interface and its subclasses to support formatting for prompt and completion content. 4. Rolled back parts of the code and dependencies from commit ca843e8. Signed-off-by: tingchuan.li <[email protected]>
c485721
to
ef2ad4d
Compare
@jonatan-ivanov Could you take a look a this PR? Thanks! |
It would help in reviewing this if you could share a link to the relevant parts of Langfuse's documentation that specifies what they're expecting for the desired integration. How likely is it to change? This is reintroducing using the deprecated API on OTel Spans to add Event Attributes, which was removed because it's deprecated and the suggested alternative was Log Events. |
https://langfuse.com/docs/integrations/spring-ai Langfuse doc still uses |
name = "chatModelPromptContentObservationTraceHandler") | ||
@ConditionalOnProperty(prefix = ChatObservationProperties.CONFIG_PREFIX, name = "trace-prompt", | ||
havingValue = "true") | ||
TracingAwareLoggingObservationHandler<ChatModelObservationContext> chatModelPromptContentObservationTraceHandler( |
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.
Why are these wrapped into TracingAwareLoggingObservationHandler
?
Please check what it does, it is used to make the tracing data available for log entries (log correlation). If I understand correctly, this is not needed in this use-case.
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.
Yes, the wrapping of TracingAwareLoggingObservationHandler
should be removed.
@@ -61,7 +61,7 @@ | |||
|
|||
<dependency> | |||
<groupId>io.micrometer</groupId> | |||
<artifactId>micrometer-tracing</artifactId> | |||
<artifactId>micrometer-tracing-bridge-otel</artifactId> |
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.
spring-ai-commons should not depend on a bridge (implementation instead of the abstraction).
} | ||
|
||
@Nullable | ||
public static Span extractOtelSpan(@Nullable TracingObservationHandler.TracingContext tracingContext) { |
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 think we should depend on the abstraction (tracing API) instead of a single implementation (OTel API/SDK).
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.
got it
@@ -60,7 +60,7 @@ | |||
|
|||
<dependency> | |||
<groupId>io.micrometer</groupId> | |||
<artifactId>micrometer-tracing</artifactId> | |||
<artifactId>micrometer-tracing-bridge-otel</artifactId> |
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.
spring-ai-model should not depend on a bridge (implementation instead of the abstraction).
.getRequired(TracingObservationHandler.TracingContext.class); | ||
Span currentSpan = TracingHelper.extractOtelSpan(tracingContext); | ||
if (currentSpan != null) { | ||
currentSpan.addEvent(AiObservationEventNames.CONTENT_COMPLETION.value(), |
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.
It seems this API will be deprecated (maybe already is?), see: micrometer-metrics/micrometer#5238 (comment)
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 wonder if it would be possible to write directly to Span.Tag
.
like
tracingContext.getSpan().tagOfStrings(AiObservationAttributes.CONTENT_COMPLETION.value(), completion);
As I’ve tested, Langfuse properly captures the data when using this approach.
This makes me a bit confused. Why are we doing this if one is already deprecated and another is not supported? |
Basically my view is that our observability needs to support products like langfuse. How to achieve this goal. wrt to this PR
that change in behavior can't occur in 1.0.x - and not sure about the strategy for 1.1 on this front. Hoping to use this issue to drive a strategy. I thought there is flexibility to override behavior - we can perhaps create ready to use repos for products that customize observability in our https://github.com/orgs/spring-ai-community/repositories org @ThomasVitale suggestions on how to structure the convo and go forward - a working google doc would seem to be a good start. |
Also related discussion is here - #3151 (comment) |
In my opinion, if the intention is to prevent large amount of data within span, constraints should be imposed through parameters to limit the length/byte size rather than directly removing these two fields from the span. |
Given the lack of stability around what to send in what format to what observability backend, I think it would be most maintainable and usable to make separate modules that customize the obserability data in the way that a specific use case needs for well-known use cases, such as Langfuse. I think this is what @markpollack suggested in his previous comment. From the user's perspective this looks like adding a dependency on a Spring AI module when they want to ship data in the way Langfuse expects. From a maintainability perspective, it means we don't need to keep making changes to core modules when things change in Langfuse. This should all be possible with auto-configuring different ObservationHandler implementations, which is what this PR does. In fact, any user can configure an implementation of ObservationHandler that does this or anything else. So it's already possible for users to achieve what this PR is doing without changes in Spring AI, right? I agree it would be best if it were easier to achieve, and that's the reason for a module that does what's needed that people can use. I suspect there are other integrations out there for AI observability and what they expect may be different than Langfuse and may change at different times and in different ways than other integrations. Trying to make the observability workable with everything from core modules sounds like a losing proposition. If there were standardization and stability around this, it would be feasible to do this from common modules, but it seems weren't not there yet. Perhaps an alternative is to make the separate module based around OTel semantic conventions for Gen AI and put the onus on observability backends to support the latest version of that (I would suspect there's some pressure for this anyway). That said, my reading of the linked documentation to Langfuse is that they intend to support what Spring AI produces, so we may not need to try so hard to send observability data in the way we used to just because that is what Langfuse supported (because that's what we produced... notice the chicken and egg problem). |
I agree with @shakuzen suggested in his previous comment, it is a better solution.. In fact, this is exactly how I've implemented it in my production code I’ll create an issue in the spring-ai-community repository at https://github.com/orgs/spring-ai-community/repositories. |
This commit refactors the content observation mechanism by adding support for trace recording without altering the original logging functionality. The motivation for this change is to ensure complete context availability in scenarios such as integration with Langfuse, which is essential for proper functionality.
Main changes:
trace-prompt
: Enables recording prompts to tracetrace-completion
: Enables recording completions to tracetrace-prompt-size
: Limits the length of prompt contextcontent-formatter
: Handles content formatting, currently supporting both 'text' and 'langfuse' modes