Skip to content

Normalize advisor observation contextual names - #6788

Open
AzazelSensei wants to merge 2 commits into
spring-projects:mainfrom
AzazelSensei:fix/observation-contextual-name-6750
Open

Normalize advisor observation contextual names#6788
AzazelSensei wants to merge 2 commits into
spring-projects:mainfrom
AzazelSensei:fix/observation-contextual-name-6750

Conversation

@AzazelSensei

Copy link
Copy Markdown

Summary

  • Strip whitespace from advisor names in DefaultAdvisorObservationConvention.getContextualName() before camel-case concatenation
  • Prevents stray spaces in span names such as tool _calling for Tool Calling Advisor
  • Adds a focused unit test covering the Tool Calling Advisor case

Fixes #6750

Test plan

  • ./mvnw -pl spring-ai-client-chat -am -Dtest=DefaultAdvisorObservationConventionTests -Dsurefire.failIfNoSpecifiedTests=false test
  • Verify OTLP / Tempo span name is tool_calling with tool callbacks enabled

Whitespace in advisor display names was preserved through camel-case
splitting, producing span names like "tool _calling ". Strip whitespace
before concatenating so contextual names stay identifier-shaped.

Fixes spring-projects#6750

Signed-off-by: Abdullah <AzazelSensei@users.noreply.github.com>
@kycasdzxc

Copy link
Copy Markdown

Ran the reported input through ParsingUtils.CAMEL_CASE to check the surrounding behaviour, and two things came up that might be worth folding in.

StructuredOutputValidationAdvisor hits the same bug and isn't covered.

It is the other advisor in the codebase that overrides getName() with a display string:

// StructuredOutputValidationAdvisor.java:109
return "Structured Output Validation Advisor";

On main that produces:

splitCamelCaseToLower -> ["structured ", "output ", "validation ", "advisor"]
join("_")             -> "structured _output _validation _advisor"
.replace("_advisor")  -> "structured _output _validation "

This PR fixes it correctly (structured_output_validation), so this is only a suggestion to add it to the new test. It is the second of the two real instances, and pinning both makes the regression boundary explicit.

The _around_advisor branch is still untested.

The only contextual-name test on main is contextualName() with "MyName" -> "my_name", which reaches neither replace. The test added here is the first to exercise _advisor. Since trimAllWhitespace changes what reaches both, pinning _around_advisor as well would be cheap.

One limitation worth recording, though I do not think it needs to block this: trimAllWhitespace only recovers an identifier when every word is capitalized. "Simple Logger advisor" becomes simple_loggeradvisor: no stray spaces, so the reported bug is fixed, but the suffix no longer strips. This is not a regression, since main gives simple _logger advisor for the same input, equally unstripped. It is the same assumption in a narrower form, in that getContextualName() expects Advisor#getName() to be identifier-shaped. That is the open question from #6750, and this change does not settle it either way, so it is likely a maintainer call.

@kycasdzxc kycasdzxc left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reworded my earlier notes as suggestions so they're easier to apply. Test-only, and entirely up to you whether to take them.

Cover Structured Output Validation Advisor whitespace normalization
and AroundAdvisor suffix stripping.

See spring-projects#6750

Signed-off-by: Abdullah <AzazelSensei@users.noreply.github.com>

Co-authored-by: kycasdzxc <105793579+kycasdzxc@users.noreply.github.com>
@cursor
cursor Bot force-pushed the fix/observation-contextual-name-6750 branch from b4302c6 to 4ddcb74 Compare August 12, 2026 22:33
@AzazelSensei

Copy link
Copy Markdown
Author

Thanks for the suggestions. Pushed the two extra tests (Structured Output Validation Advisor + AroundAdvisor suffix).

@kycasdzxc

kycasdzxc commented Aug 12, 2026

Copy link
Copy Markdown

@AzazelSensei Glad they were useful. Since the suggestions were applied manually,
the co-author trailer didn't get added automatically. Could you amend
the commit to include it?

Co-authored-by: kycasdzxc <105793579+kycasdzxc@users.noreply.github.com>

@AzazelSensei

Copy link
Copy Markdown
Author

Amended — added your Co-authored-by.

@cursor
cursor Bot force-pushed the fix/observation-contextual-name-6750 branch from 4ddcb74 to 937f811 Compare August 12, 2026 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DefaultAdvisorObservationConvention produces a span name with stray spaces

3 participants