add ChatModelSpanContributor #1260

flyinfish · 2025-02-03T07:20:07Z

enabling adding more/custom attributes to span, namely before span is closed

for onRequest this would already be possible just using another ChatModelListener. not so unfortunately for onResponse as span is ended in SpanChatModelListener.onResponse() and so additional attributes land on parent-span overwriting each other with every new request.

this change is just missing some Processor Magic producing a default-ExtendedSpanChatModelListener and some docs may-be.

geoand · 2025-02-03T07:23:00Z

Interesting!

I will have a closer look in a few days as I am traveling

...rc/main/java/io/quarkiverse/langchain4j/runtime/listeners/ExtendedSpanChatModelListener.java

flyinfish · 2025-02-04T07:27:05Z

@geoand @brunobat
code should be ok like this!?, still missing docs but let's agree first if you like to have it merged

flyinfish · 2025-02-04T07:28:17Z

.../quarkiverse/langchain4j/test/listeners/ListenersProcessorOnlySpanChatModelListenerTest.java

+            .setArchiveProducer(
+                    () -> ShrinkWrap.create(JavaArchive.class)
+                            .addClasses(AiService.class, EchoChatLanguageModelSupplier.class))
+            .setForcedDependencies(List.of(Dependency.of("io.quarkus", "quarkus-opentelemetry", "3.15.2")));


@geoand
"3.15.2" any idea where this is supposed to come from?
or is there a simpler solution to it adding otel?

I'll need to externalize the version, but just go ahead and copy it for now if you need to add a new test.

geoand · 2025-02-04T08:21:25Z

How about using @All ListChatModelSpanContributor> as the injection point.

so we can support multiple contributors? That way we also won't need the default bean.

...untime/src/main/java/io/quarkiverse/langchain4j/runtime/listeners/SpanChatModelListener.java

brunobat · 2025-02-04T09:37:11Z

By principal, we should own the list of attributes to be placed in a span, to make sure we have all the important attributes covered.
Custom ad-hock attribute names, out of conventions, will never be considered by APM tools and users will have to handle their schema themselves.
Saying this, if ppl really want the feature, we should allow it with a warning.

geoand · 2025-02-04T10:28:10Z

if ppl really want the feature, we should allow it with a warning.

I don't think this is necessary. We can add the proper documentation and Javadoc warning about this.

flyinfish · 2025-02-04T17:04:18Z

.../quarkiverse/langchain4j/test/listeners/ListenersProcessorOnlySpanChatModelListenerTest.java

+        aiService.test();
+
+        await().untilAsserted(() -> assertThat(exporter.getFinishedSpanItems()).hasSize(1));
+        assertThat(exporter.getFinishedSpanItems()).extracting(SpanData::getName)


@geoand this will get nasty
i have yet to understand how spans are built in test - do you wanna go this road?
or may be a more stable / Mockito approach

I guess for the purposes of this test, we can go with a mock approach (although I'm not a huge fan)

code should be ready, still missing version to be externalized
there is one "bug" i think i fixed that span was not ended in onError

code should be ready, still missing version to be externalized

I'll do that after the PR is merged

flyinfish · 2025-02-05T09:30:00Z

By principal, we should own the list of attributes to be placed in a span, to make sure we have all the important attributes covered. Custom ad-hock attribute names, out of conventions, will never be considered by APM tools and users will have to handle their schema themselves. Saying this, if ppl really want the feature, we should allow it with a warning.

@brunobat this solution enables different attribute-sets for every "APM worth having" to be collected. they could be enabled by config

geoand

Very nice! I only added a few very minor comments

geoand · 2025-02-05T10:35:53Z

...untime/src/main/java/io/quarkiverse/langchain4j/runtime/listeners/SpanChatModelListener.java

@@ -53,6 +58,7 @@ public void onRequest(ChatModelRequestContext requestContext) {
        var attributes = requestContext.attributes();
        attributes.put(OTEL_SCOPE_KEY_NAME, scope);
        attributes.put(OTEL_SPAN_KEY_NAME, span);
+        chatModelSpanContributors.forEach(c -> invokeSafeWithRecordException(span, () -> c.onRequest(requestContext, span)));


Let's please avoid lambdas in this code. We generally strive for this principle in code that is executed at runtime

np... - but how comes?

...untime/src/main/java/io/quarkiverse/langchain4j/runtime/listeners/SpanChatModelListener.java

...ime/src/main/java/io/quarkiverse/langchain4j/runtime/listeners/ChatModelSpanContributor.java

flyinfish · 2025-02-05T13:26:42Z

docs/modules/ROOT/pages/ai-services.adoc


-== AI method declaration
+=== AI method declaration


@geoand current doc broken here?

Looks like it!

flyinfish · 2025-02-05T13:28:14Z

hope i am done!?

enabling users adding more/custom attributes/data to span enabling us to add pre-built attribute-sets for diffrent APMs Closes quarkiverse#1256

geoand

Thanks a lot!

quarkus-bot · 2025-02-05T16:07:26Z

Status for workflow `Build (on pull request)`

This is the status report for running Build (on pull request) on commit 73af979.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

flyinfish requested a review from a team as a code owner February 3, 2025 07:20

flyinfish marked this pull request as draft February 3, 2025 07:20

flyinfish mentioned this pull request Feb 3, 2025

Enhance Tracing attributes #1256

Closed

geoand reviewed Feb 3, 2025

View reviewed changes

...rc/main/java/io/quarkiverse/langchain4j/runtime/listeners/ExtendedSpanChatModelListener.java Outdated Show resolved Hide resolved

flyinfish force-pushed the 1256 branch 3 times, most recently from 4b3e886 to d98170f Compare February 4, 2025 07:21

flyinfish commented Feb 4, 2025

View reviewed changes

flyinfish changed the title ~~draft: add extended-span-chat-model-listener~~ draft: add ChatModelSpanContributor Feb 4, 2025

brunobat suggested changes Feb 4, 2025

View reviewed changes

...untime/src/main/java/io/quarkiverse/langchain4j/runtime/listeners/SpanChatModelListener.java Outdated Show resolved Hide resolved

flyinfish force-pushed the 1256 branch 3 times, most recently from 4f9203a to 8d7bc45 Compare February 4, 2025 17:02

flyinfish commented Feb 4, 2025

View reviewed changes

flyinfish force-pushed the 1256 branch from 8d7bc45 to 4703fac Compare February 5, 2025 09:25

geoand reviewed Feb 5, 2025

View reviewed changes

flyinfish force-pushed the 1256 branch from 4703fac to ee6a4ba Compare February 5, 2025 13:25

flyinfish commented Feb 5, 2025

View reviewed changes

flyinfish changed the title ~~draft: add ChatModelSpanContributor~~ add ChatModelSpanContributor Feb 5, 2025

flyinfish marked this pull request as ready for review February 5, 2025 13:30

This comment has been minimized.

Sign in to view

flyinfish force-pushed the 1256 branch from ee6a4ba to 2b81e5f Compare February 5, 2025 14:44

This comment has been minimized.

Sign in to view

flyinfish force-pushed the 1256 branch from 2b81e5f to 3f0a0ba Compare February 5, 2025 15:07

add ChatModelSpanContributor

73af979

enabling users adding more/custom attributes/data to span enabling us to add pre-built attribute-sets for diffrent APMs Closes quarkiverse#1256

flyinfish force-pushed the 1256 branch from 3f0a0ba to 73af979 Compare February 5, 2025 15:28

This comment has been minimized.

Sign in to view

geoand approved these changes Feb 5, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

geoand merged commit 97cb976 into quarkiverse:main Feb 5, 2025
72 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ChatModelSpanContributor #1260

add ChatModelSpanContributor #1260

flyinfish commented Feb 3, 2025

geoand commented Feb 3, 2025

flyinfish commented Feb 4, 2025

flyinfish Feb 4, 2025

geoand Feb 4, 2025

geoand commented Feb 4, 2025 •

edited

Loading

brunobat commented Feb 4, 2025

geoand commented Feb 4, 2025

flyinfish Feb 4, 2025

geoand Feb 4, 2025

flyinfish Feb 5, 2025

geoand Feb 5, 2025

flyinfish commented Feb 5, 2025

geoand left a comment

geoand Feb 5, 2025

flyinfish Feb 5, 2025

flyinfish Feb 5, 2025

geoand Feb 5, 2025

flyinfish commented Feb 5, 2025

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

geoand left a comment

This comment has been minimized.

quarkus-bot bot commented Feb 5, 2025

add ChatModelSpanContributor #1260

add ChatModelSpanContributor #1260

Conversation

flyinfish commented Feb 3, 2025

geoand commented Feb 3, 2025

flyinfish commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoand commented Feb 4, 2025 • edited Loading

brunobat commented Feb 4, 2025

geoand commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyinfish commented Feb 5, 2025

geoand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyinfish commented Feb 5, 2025

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

geoand left a comment

Choose a reason for hiding this comment

This comment has been minimized.

quarkus-bot bot commented Feb 5, 2025

Status for workflow Build (on pull request)

geoand commented Feb 4, 2025 •

edited

Loading

Status for workflow `Build (on pull request)`