Skip to content

Remove possible large amount of data from spans #3003

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

Closed
wants to merge 1 commit into from

Conversation

jonatan-ivanov
Copy link
Member

@jonatan-ivanov jonatan-ivanov commented May 6, 2025

This removes the possible large amount of data that was attached
to spans and it logs the data out. This change also removes the direct
dependency on the OTel SDK.

@markpollack
Copy link
Member

@ThomasVitale thoughts?

@markpollack
Copy link
Member

markpollack commented May 7, 2025

No dashboards should be effected.
The original API used is now deprecated - open-telemetry/opentelemetry-specification#4430

@ThomasVitale
Copy link
Contributor

ThomasVitale commented May 8, 2025

The OTel SDK dependency is optional, so it won't be forced by Spring AI. It's only there to support a feature not supported by Micrometer, and it's only enabled if the developer actively configures OpenTelemetry in their application. Could we keep it?

The Client API for Span Events has been deprecated (though, it won't be removed from the SDK), but the Backend API is still there. In the OpenTelemetry SDK, the Logging API is now used to export both log events and span events. Since Micrometer doesn't support either (log events, span event attributes), the only solution is using the OpenTelemetry API directly. So actually the SDK is not needed. It's enough to bring the API.

Prompts and completions can be very big, and span attributes are not really suitable for that if we consider the performance point of view (I'm not aware of backends providing good support for this, except the specialised LLM Observability backends). But my main concern is the user experience. It will be very hard to visualize a trace if prompts and completions are included as span attributes. The fallback is there only because Zipkin doesn't support span event attributes. Considering this feature is primarily meant to be used during development for continuous inspection of what LLMs do under the hood, it will result in a suboptimal experience.

In any case, if it's not ok to have an OTel dependency, I'm available to provide the community with an alternative when upgrading, so they don't have to re-implement the same thing if they want to continue using this feature.

@markpollack markpollack self-assigned this May 8, 2025
@asaikali
Copy link

asaikali commented May 8, 2025

@ThomasVitale can you elaborate on what you mean by "In the OpenTelemetry SDK, the Logging API is now used to export both log events and span events." does this mean that you attach a span event attribute but the exporter sends it the logging backend instead of the tracing backend?

@shakuzen
Copy link
Member

shakuzen commented May 9, 2025

does this mean that you attach a span event attribute but the exporter sends it the logging backend instead of the tracing backend?

See the deprecation plan: https://github.com/open-telemetry/opentelemetry-specification/pull/4430/files#diff-e3aefa35ffa8190a508324a37cd3b434ea73fb0a684f09434cad682665993345R92-R115

There MUST be a way to send (log-based) exceptions and Events as Span Events for use cases that rely on Span Events being emitted in the same proto envelope as their containing span, and for users who need to preserve the prior behavior when updating to new instrumentation.

It will be possible to enable (but it will be disabled by default, if I understand correctly) Log Events being forwarded by the SDK (or Collector) to Spans, not the other way around.

Prompts and completions can be very big, and span attributes are not really suitable for that

I generally agree with this (though I don't understand why span event attributes are more suitable other than some tracing UIs giving them different UX than span attributes). My suggestion would be to remove the prompts/completions from ending up on Spans at all and instead they should be emitted via logs (as an opt-in feature due to privacy and performance issues). That seems to be the direction OTel is pushing as well. With log correlation, it should be easy to view the logs with the prompts/completions from the tracing UI, and it will be even easier when running the application locally compared to being in the span data.

@ThomasVitale
Copy link
Contributor

@shakuzen I like the logs option more than span attributes as the default way to export prompt content and completion. And we could perhaps consider the span attribute/events option separately in the context of conventions and OTel support.

If we go with that, we could even phase out the custom SimpleLoggerAdvisor in favour of an Observation-based approach.

This removes the possible large amount of data that was attached
to spans and it logs the data out. This change also removes the direct
dependency on the OTel SDK.
@jonatan-ivanov
Copy link
Member Author

For the sake of documentation: we had some discussions in the background, and it seems everyone is in agreement that we should remove the prompts(+completions, etc.) from the Spans (both as normal attributes and span event attributes) and emit logs instead (opt-in).

I introduced these changes, there are a few things still missing I'm planning to add over the weekend:

  • tests for the trace aware handler
  • tests for auto-configuration
  • update docs

@jonatan-ivanov jonatan-ivanov changed the title Remove direct dependency to the OTel SDK Remove possible large amount of data from spans May 10, 2025
@tzolov
Copy link
Contributor

tzolov commented May 10, 2025

Rebased, additionally update the reference documentation and minor property name changes.
Merged at ca843e8

@tzolov tzolov closed this May 10, 2025
@jonatan-ivanov jonatan-ivanov deleted the de-otel branch May 12, 2025 22:59
jonatan-ivanov added a commit to jonatan-ivanov/spring-ai that referenced this pull request May 15, 2025
Some tests were missing from spring-projectsgh-3003, this change adds them

See spring-projectsgh-3003
tzolov pushed a commit that referenced this pull request May 15, 2025
Some tests were missing from gh-3003, this change adds them

See gh-3003
jonatan-ivanov added a commit to jonatan-ivanov/spring-ai that referenced this pull request May 16, 2025
The features are disabled by default and users need to explicitly
enable them. Probably it makes more sense to log on INFO level so that
users don't need to change the log level if they use the default
behavior of Spring Boot.

See spring-projectsgh-3003
See spring-projectsgh-3127
tzolov pushed a commit that referenced this pull request May 17, 2025
The features are disabled by default and users need to explicitly
enable them. Probably it makes more sense to log on INFO level so that
users don't need to change the log level if they use the default
behavior of Spring Boot.

See gh-3003
See gh-3127
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.

None yet

6 participants