Adding opt-in option for the debug endpoint in ADK REST API - #1413

Re-reviewed after the two follow-up commits. Moving the opt-in into its own DebugApiConfig rather than renaming the existing type is the right call and it does resolve the compatibility break — I re-ran a third-party program that constructs adkrest.ServerConfig{DebugConfig: adkrest.DebugTelemetryConfig{TraceCapacity: 5000}} and it now builds against this branch exactly as it does against main, and apidiff reports only DebugApiConfig: added and ServerConfig.DebugApiConfig: added. Nothing below blocks. A few notes are inline, and two things are worth a decision before this lands.

The gate also removes a route that is not named "debug"

DebugAPIRouter owns three routes, and the third is /apps/{app_name}/users/{user_id}/sessions/{session_id}/events/{event_id}/graph. It carries no /debug prefix and sits in the ordinary apps/sessions tree, so a caller would not associate it with a flag called include_debug_api. Driving NewServer and ServeHTTP at this commit:

examples/rest/main.go#L62-L66 builds a ServerConfig with no debug config, so the repository's own example loses all three. As far as I can tell nothing in-tree actually calls the graph endpoint, so this may well be fine — I mainly want it to be a decision rather than a side effect, and worth a line in the description either way.

Nothing tests the gate

Two edits against the suite: deleting the if at handler.go#L59 so the debug router always registers again, and inverting it to if !cfg.DebugApiConfig.IncludeDebugAPI. Both leave go test ./server/adkrest/... green, so this feature could be removed or turned inside out without a test noticing. No test in the tree constructs a non-zero debug config, and the tests in controllers/debug_test.go call the handlers directly rather than through mux, so they cannot observe registration.

Two table cases on NewServer would close it: default config gives 404 on all three patterns, IncludeDebugAPI: true gives non-404.

One for later, not for this PR

api.go#L95-L96 registers the span and log processors unconditionally, and NewServer builds the telemetry store before the gate, so with the flag off the process still captures every span into a buffer nothing can read. That is unchanged from main — this PR adds no retention — but it does remove the only reader, so in the default configuration the cost now buys nothing. Better as a follow-up than as scope added here.

One thing I checked that turned out fine: the default flip does not break the trace UI. The bundled Web UI calls /dev/apps/{app}/debug/trace/... and no Go route serves a /dev prefix, so those calls already 404 on main.

Re-checked at 1abb4fa4. All four inline points from the previous round are addressed — the type is DebugAPIConfig, debugAPI has the initialism, and the --debug_api validation now runs before os.MkdirTemp so the rejected-argument path no longer leaves a temp directory behind. Build, the full suite, vet, gofmt and apidiff are all clean at this commit, and apidiff reports additive changes only.

Two things left inline, both small and both worth doing before this merges rather than after. One of them is a consequence of the wording change I asked for last round, so apologies for the round trip.

Still open from the previous round, unchanged and not re-argued here: the …/events/{event_id}/graph route is still gated off by default along with the two /debug/trace routes, and there is still no test that would notice if the gate were removed or inverted. Both are described in the earlier review.

Re-checked at 50e2ed0b — the field rename and the --api correction both look right, and I confirmed --api --debug_api now parses cleanly with a2a_agent_url untouched. Build, suite, vet, gofmt, staticcheck and apidiff are all clean.

One design question left, plus two small things inline.

The event graph route is gated along with the trace routes

DebugAPIRouter owns three routes and the gate covers all of them:

GET /debug/trace/{event_id}
GET /debug/trace/session/{session_id}
GET /apps/{app_name}/users/{user_id}/sessions/{session_id}/events/{event_id}/graph

The third has no /debug in its path and sits in the ordinary apps/sessions tree, so a caller has no way to associate it with a flag named include_debug_api. Measured at this commit through NewServer and ServeHTTP, the graph route goes from 400 on main to 404 by default here, while /health stays 200 as a control. examples/rest/main.go#L62-L66 sets no debug config, so the repo's own example is in that group.

Worth saying plainly: apidiff cannot catch this one. The break is behavioural, not source-level, so a caller who never named the config type still compiles and simply starts getting 404s.

Three ways to go, and I do not think any of them is obviously right:

1. Document the coupling. Keep the gate as it is and say what it covers — a doc comment on the field naming all three routes (suggested text inline), a corrected flag description, and a line in the release notes. Cheapest, and the notes are the part that actually reaches an existing caller, since nobody reads godoc for a field they do not know exists.
2. Move the graph route off DebugAPIRouter. Right if it is ordinary API surface that happens to be implemented by the debug controller. Makes the question disappear instead of documenting it.
3. Leave it as is. Defensible — nothing in-tree reaches that route, and the bundled Web UI cannot either, since it calls /dev/apps/… and no Go route serves a /dev prefix. Even then the release note still matters for callers outside this repo.

Option 1 is what I would pick, but it is your call and option 2 is a reasonable read of what the endpoint is.

Two doc comments that now describe something that is off by default

SpanProcessor and LogProcessor both tell callers the processors capture data "used for /debug/trace endpoint of the ADK REST API server", and invite registering them on the application's own provider. With the new default that endpoint is not registered, so a library caller who follows the comment feeds a store nothing can read. Both sit outside the diff, so they are here rather than on the lines.

Looks good to me.

I re-ran the new gate test against two deliberate breakages to check it earns its place: deleting the if so the debug router always registers, and inverting it. Both make TestNewServerDebugAPIGate fail, so the test genuinely pins the behaviour rather than just executing it. Coverage on server/adkrest also goes from 78.6% to 82.4%, and apidiff reports additive changes only.

One thing before merging: the branch is behind main and will need a merge.

The unconditional span and log processor registration is still worth a follow-up at some point — that predates this PR and is not something to grow this change with.