fix(sync): wire metrics handler into expand ProgressLog#862
Open
arreyder wants to merge 2 commits into
Open
Conversation
NewProgressCounts is constructed without progresslog.WithMetricsHandler, so the four expand instruments — baton.sync.expand.actions_remaining, .actions_burned, .decompressed_bytes, .decompressed_bytes_delta — were emitted into the default no-op handler and never reached an exporter. The structured "Expanding grants" logs still fired, but the OTel path introduced in #804 was dead from the consumer's perspective. Add a WithMetricsHandler SyncOpt that forwards the caller's pre-tagged handler into the ProgressLog, and apply it in NewSyncer. Per the WithMetricsHandler doc on progresslog.go:149-151, the caller owns the tagging (tenant_id / connector_id) — baton-sdk has no view of those. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
General PR Review: fix(sync): wire metrics handler into expand ProgressLogBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commits add a nil guard to Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
… doc From code review of the previous commit: 1. Add a syncer-level test that constructs a syncer with WithMetricsHandler and asserts the caller's handler receives the actions_remaining gauge. Without it, dropping the conditional append in NewSyncer would silently disable every consumer's baton.sync.expand.* metrics — log assertions in existing tests would still pass. 2. Guard nil in the WithMetricsHandler SyncOpt closure to match sibling options (WithTransitionHandler, WithProgressHandler) and the wrapped progresslog.WithMetricsHandler. Prevents a stray WithMetricsHandler(nil) later in the options list from clobbering a previously-set handler. 3. Fix a stale instrument name in the WithMetricsHandler doc on the progresslog side: decompressed_bytes_growth → decompressed_bytes_delta (matches the actual constant at progresslog.go:31). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
progresslog.NewProgressCountsis constructed inpkg/sync/syncer.go:2849withoutWithMetricsHandler, so the four expand OTel instruments added in feat(sync): emit OTel metrics for grant-expansion progress (CE-702) #804 (baton.sync.expand.actions_remaining,.actions_burned,.decompressed_bytes,.decompressed_bytes_delta) emit to the default no-op handler and never reach an exporter. Structured "Expanding grants" logs still fire; only the metric path is dead.WithMetricsHandlerSyncOptso callers can inject a pre-taggedmetrics.Handler, and forward it intoprogressLogOptsbeforeNewProgressCounts.progresslog.WithMetricsHandler(pkg/sync/progresslog/progresslog.go:144-158), the caller owns tagging (tenant_id/connector_id); baton-sdk does not see those identifiers, so this PR pipes the handler through without decoration.Why this matters
During long grant expansions, operators currently have to scrape log timestamps to answer "is this connector burning through actions or wedged?" — the same workflow #804 was meant to obsolete. Once consumers (e.g. the main platform repo) pass their existing sync-scoped handler via
sync.WithMetricsHandler(h), the gauges/counters land in Datadog and dashboards/alerts become possible.Test plan
go build ./...go test ./pkg/sync/...golangci-lint run ./pkg/sync/...— no new findings (the 3 pre-existing issues inpkg/sync/expandare untouched by this diff)baton.sync.expand.actions_remainingshows up in Datadog during a grant-expansion step on a connector with grants