Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions pkg/tapper/invocation_telemetry.go
Original file line number Diff line number Diff line change
Expand Up @@ -242,10 +242,24 @@ func (r *httpInvocationReporter) send(ctx context.Context, batch []InvocationEve
}
}

// disablesInvocationTelemetry reports whether a status means "stop trying" as
// opposed to "try again later". Every code here says the hub will never accept
// this client's events, so retrying only wastes requests.
//
// 400 is in the list because a hub older than the client rejects any field it
// does not know — its decoder disallows unknown fields — and the client cannot
// negotiate the payload down. Without this, a tap carrying a newly added field
// would re-send a guaranteed-rejected batch on every flush for the life of the
// process. Degrading to no telemetry is the correct outcome, and it is what
// lets the client and the hub release in either order.
//
// 413 is deliberately absent: batch contents vary, so a too-large batch says
// nothing about the next one.
func disablesInvocationTelemetry(status int) bool {
switch status {
case http.StatusUnauthorized, http.StatusForbidden, http.StatusNotFound,
http.StatusMethodNotAllowed, http.StatusGone, http.StatusNotImplemented:
case http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden,
http.StatusNotFound, http.StatusMethodNotAllowed, http.StatusGone,
http.StatusNotImplemented:
return true
default:
return false
Expand Down
22 changes: 22 additions & 0 deletions pkg/tapper/invocation_telemetry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,28 @@ func TestHTTPInvocationReporterTimeoutAndOlderHubAreBestEffort(t *testing.T) {
r.Close(ctx)
require.Equal(t, int64(1), calls.Load())
})

// A hub older than this client rejects any field it does not know, and the
// client cannot negotiate the payload down. Retrying a guaranteed-rejected
// batch on every flush is pure waste, so 400 stops the process reporter the
// same way an unsupported endpoint does.
t.Run("rejected payload disables process reporter", func(t *testing.T) {
var calls atomic.Int64
client := &http.Client{Transport: telemetryRoundTripFunc(func(_ *http.Request) (*http.Response, error) {
calls.Add(1)
return &http.Response{StatusCode: http.StatusBadRequest, Body: io.NopCloser(strings.NewReader("")), Header: make(http.Header)}, nil
})}
r := newHTTPInvocationReporter("https://old.example.com/api/v1/telemetry/invocations", "token", "test", invocationReporterOptions{
client: client, batchSize: 1, flushInterval: time.Hour,
})
r.Report(InvocationEvent{Surface: "mcp", Tool: "first"})
require.Eventually(t, r.disabled.Load, time.Second, time.Millisecond)
r.Report(InvocationEvent{Surface: "mcp", Tool: "second"})
ctx, cancel := context.WithTimeout(context.Background(), time.Second)
defer cancel()
r.Close(ctx)
require.Equal(t, int64(1), calls.Load(), "the rejected batch must not be retried")
})
}

func TestHTTPInvocationReporterConcurrentReportAndClose(t *testing.T) {
Expand Down
Loading