Skip to content

Create StreamingTraceManager once#107

Merged
krisztianfekete merged 3 commits intoagentevals-dev:mainfrom
ajimenez1503:feat/StreamingTraceManager
Apr 7, 2026
Merged

Create StreamingTraceManager once#107
krisztianfekete merged 3 commits intoagentevals-dev:mainfrom
ajimenez1503:feat/StreamingTraceManager

Conversation

@ajimenez1503
Copy link
Copy Markdown
Contributor

Summary

Fixes Issue #99 by creating StreamingTraceManager once during startup and injecting it explicitly into the live server components instead of discovering it through module-level app state.

  • Add create_app(...) and create_otlp_app(...) factories so the main API app and OTLP HTTP app can receive the shared manager directly.
  • Update cli._run_servers() to create a single StreamingTraceManager and pass it to the main API app, OTLP HTTP app, and OTLP gRPC server.
  • Remove the old CLI dependency on require_trace_manager_from_app(...).
  • Refactor integration fixtures to build live apps from the new factories instead of relying on import-time side effects.
  • Add a focused CLI startup wiring test to verify all live server surfaces share the same manager instance.

Why

  • Removes hidden cross-module coupling between the main app, OTLP HTTP app, and CLI startup.
  • Makes startup ownership explicit as the live server surface grows.
  • Preserves the existing route contract of reading the manager from app.state.trace_manager.

@ajimenez1503 ajimenez1503 force-pushed the feat/StreamingTraceManager branch from 0de5e6b to ee46324 Compare April 6, 2026 11:58
Comment on lines +560 to +561
main_server = uvicorn.Server(uvicorn.Config(main_app, port=port, **shared_kwargs))
otlp_http_server = uvicorn.Server(uvicorn.Config(otlp_app, port=otlp_http_port, **shared_kwargs))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this break hot reload functionality of --dev?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch.

I checked this more closely and the combined serve --dev path wasn't actually getting uvicorn hot reload before this change either: it runs uvicorn.Server(...).serve() directly, and the reload=True / reload_dirs=... config there doesn't start uvicorn's reload supervisor. So this PR doesn't introduce a new regression, but it did make the mismatch more visible. Could you confirm?

I've cleaned that up in this branch by removing the dead reload plumbing and keeping the startup wiring explicit around the shared StreamingTraceManager. If we want real hot reload for the multi-surface dev server, I'd prefer to handle that as a follow-up since it likely needs a different startup shape. Make sense?

*,
trace_manager: StreamingTraceManager | None = None,
enable_streaming: bool = False,
manage_trace_manager_lifecycle: bool = True,
Copy link
Copy Markdown
Contributor

@krisztianfekete krisztianfekete Apr 6, 2026

Choose a reason for hiding this comment

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

Nothing in this PR sets this to False, and the guard already makes this a no-op when there's no manager. Do you have a use-case for this in mind?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Removed the unused lifecycle flag

@krisztianfekete krisztianfekete merged commit e14f630 into agentevals-dev:main Apr 7, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create StreamingTraceManager once

3 participants