CAMEL-23861: GenAI observability Phase 3 - Spring AI chat and camel-main config - #25507
Conversation
…ain config Extend GenAI observability to spring-ai-chat producers with OpenTelemetry span attributes and token usage recording. Add Spring AI model resolution to GenAiModelResolver and first-class camel.ai.observability.enabled support in Camel Main (typed configuration, auto-configuration, and metadata). Includes SpringAiChatObservabilityTest, AiObservabilityConfigurationPropertiesTest, upgrade guide notes, and PrepareCamelMainMojo metadata prefix fix. Co-authored-by: Cursor Agent <noreply@cursor.com>
- Do not record request model as response model on entity conversion path - Resolve model metadata from ChatClient when ChatModel is not configured - Harden GenAiModelResolver reflection when optional methods are absent Co-authored-by: Cursor Agent <noreply@cursor.com>
|
AI-generated on behalf of atiaomar1978-hub Bugbot follow-up (commit pushed)Addressed both review findings:
Please re-review when CI is green. |
- Rewrite GenAiModelResolver with reflection-only LangChain4j detection - Cache ChatModel for observability when only ChatClient is configured - Use responseEntity() for entity path to capture token usage in spans - Add camel.ai.observability prefix to Main GROUP_PREFIXES - Remove optional langchain4j-core dependency from camel-spring-ai-chat - Expand Spring AI observability tests (ChatClient-only, entity, disabled) Co-authored-by: Omar Atie <atiaomar1978-hub@users.noreply.github.com>
- Fix SpringAiChatObservabilityTest tracer wiring for isolated contexts - Regenerate camel-main catalog with camel.ai.observability.enabled key Co-authored-by: Omar Atie <atiaomar1978-hub@users.noreply.github.com>
|
AI-generated on behalf of atiaomar1978-hub Addressed Grok review findings in commits Critical
Medium
All observability tests pass locally. |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thank you for this contribution — the observability wiring and reflection-based resolver are well-designed. I have a few findings that need to be addressed.
Confirmed issues
-
Wrong configuration property prefix — The property uses
camel.ai.observability.enabled(two-level nesting) but every existing Camel Main configuration group uses a single-level prefix undercamel.:camel.health.,camel.opentelemetry2.,camel.errorRegistry.,camel.routeController., etc. The correct name should becamel.ai-observability.enabled(hyphenated, single level). This affectsPREFIX_AI_OBSERVABILITY,GROUP_PREFIXES,PrepareCamelMainMojo, the configuration properties classes, the metadata JSON, and the documentation. -
processStructuredOutputRequestdrops token usage headers — The PR replacesrequest.call().chatResponse()withcallWithObservability(request, exchange)and removes thepopulateTokenUsage(response, exchange)call. WhilecallWithObservabilityrecords tokens on the span, it does NOT populate exchange headers (INPUT_TOKEN_COUNT,OUTPUT_TOKEN_COUNT,TOTAL_TOKEN_COUNT). Compare with the entity path which calls bothrecordObservationSuccessANDpopulateTokenUsage. The default chat path works becausepopulateResponse()includespopulateTokenUsage(). This is a user-visible regression. -
FQCN violation —
org.springframework.ai.chat.client.ResponseEntity<ChatResponse, T>is used inline inprocessEntityRequestinstead of adding an import. Per project conventions: "Do NOT use fully qualified class names."
Suggestions (non-blocking)
-
Missing warning log for unconfigured AI observability properties — Every other configuration group in
BaseMainSupporthas a warning block for unconsumed properties. The PR doesn't add one. -
Fragile private field access in
extractChatModelFromClient— UsesgetDeclaredField("chatModel")+setAccessible(true)on Spring AI internals. The graceful fallback is good, but worth noting as a maintenance risk.
Note: This review covers project rules, conventions, and code patterns. It does not replace specialized review tools such as CodeRabbit, Sourcery, or SonarCloud.
This review was generated by an AI agent (Claude Code on behalf of davsclaus) and may contain inaccuracies. Please verify all suggestions before applying.
| private static final String PREFIX_TRACE = "camel.trace."; | ||
| private static final String PREFIX_ROUTE_CONTROLLER = "camel.routeController."; | ||
| private static final String PREFIX_ERROR_REGISTRY = "camel.errorRegistry."; | ||
| private static final String PREFIX_AI_OBSERVABILITY = "camel.ai.observability."; |
There was a problem hiding this comment.
The property prefix camel.ai.observability. uses two-level nesting, but every existing Camel Main configuration group uses a single-level camelCase prefix under camel.:
camel.health.,camel.opentelemetry2.,camel.errorRegistry.,camel.routeController.,camel.management., etc.
The canonical name should be camel.aiObservability.enabled (camelCase, matching e.g. camel.routeController.), with camel.ai-observability.enabled (dash-style) also accepted. The rename needs to ripple through: this constant, GROUP_PREFIXES, PrepareCamelMainMojo, AiObservabilityConfigurationProperties, metadata JSON, and docs.
| @@ -965,8 +989,6 @@ private <T> void processStructuredOutputRequest( | |||
| // Also set headers | |||
| exchange.getMessage().setHeader(SpringAiChatConstants.CHAT_RESPONSE, responseText); | |||
| exchange.getMessage().setHeader(SpringAiChatConstants.STRUCTURED_OUTPUT, structuredOutput); | |||
There was a problem hiding this comment.
The old code called populateTokenUsage(response, exchange) after this line, but that call was removed. callWithObservability() records token data on the span but does NOT populate the exchange headers (INPUT_TOKEN_COUNT, OUTPUT_TOKEN_COUNT, TOTAL_TOKEN_COUNT).
Compare with:
- The entity path (
processEntityRequest) which correctly calls bothrecordObservationSuccessANDpopulateTokenUsage - The default chat path which calls
populateResponse()(which includespopulateTokenUsage)
Please restore populateTokenUsage(response, exchange) here.
| GenAiObservationContext observationContext = buildObservationContext(); | ||
| GenAiObservation observation = GenAiObservability.start(exchange, observationContext); | ||
| try { | ||
| org.springframework.ai.chat.client.ResponseEntity<ChatResponse, T> responseEntity |
There was a problem hiding this comment.
Per project conventions: "Do NOT use fully qualified class names (FQCNs) in Java code. Always add an import statement and use the simple class name."
Since org.springframework.http.ResponseEntity is not imported in this file, a regular import for org.springframework.ai.chat.client.ResponseEntity would work without conflicts.
| org.springframework.ai.chat.client.ResponseEntity<ChatResponse, T> responseEntity | |
| ResponseEntity<ChatResponse, T> responseEntity |
3 participants
AI-generated on behalf of atiaomar1978-hub
Summary
Phase 3 of CAMEL-23861 extends GenAI observability beyond Phase 1 (langchain4j/openai) and Phase 2 (TUI usage view):
spring-ai-chat) producers now emit GenAI OpenTelemetry span attributes and record token usage viaGenAiObservability, matching the langchain4j-chat pattern.camel.ai.observability.enabled(CAMEL-24386): application.properties, programmaticmain.configure().ai().observability().withEnabled(false), and metadata/docs.Changes
SpringAiChatProducerGenAiObservabilityGenAiModelResolvercamel-mainAiConfigurationProperties/AiObservabilityConfigurationProperties, auto-config inBaseMainSupportPrepareCamelMainMojocamel.ai.observability.enabled(notcamel.main.enabled)ai-observability.adoc, upgrade guide 4.23 entry for Spring AI + Main configSpringAiChatObservabilityTest,AiObservabilityConfigurationPropertiesTest,GenAiModelResolverTest(Spring AI)Testing
./mvnw -pl components/camel-ai/camel-ai-observability-api,components/camel-ai/camel-ai-observability,components/camel-spring-parent/camel-spring-ai/camel-spring-ai-chat,core/camel-main -am test \ -Dtest=GenAiModelResolverTest,SpringAiChatObservabilityTest,AiObservabilityConfigurationPropertiesTest,GenAiObservabilityTestRelated