feat(tracing): add accounts attribute to single-account controller spans#1360
Open
sylr wants to merge 1 commit into
Open
feat(tracing): add accounts attribute to single-account controller spans#1360sylr wants to merge 1 commit into
sylr wants to merge 1 commit into
Conversation
Tag controller trace spans that operate on accounts (GetAccount, GetAggregatedBalances, GetVolumesWithBalances, SaveAccountMetadata, DeleteAccountMetadata) with an `accounts` span attribute. The same StringSlice key already emitted by bulk storage operations is reused, so a single query can surface every span involving a given account. Accounts are extracted from the query builder ($match/$in on address/account) for query-based methods and from the explicit address for metadata methods, then deduplicated. Constraint: query.Builder.Walk exposes no negation context, so $not filters are best-effort and may tag an excluded account Rejected: GetResourceQuery() interface method | rippled into shared pagination plumbing and surfaced a latent type bug in Iterate Confidence: high Scope-risk: narrow Not-tested: $not-negated address filters; partial-segment address filters on list/aggregate ops Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughThis PR enriches distributed traces with account context by introducing account extraction helpers and integrating them into five ledger controller methods. ChangesAccount-based span enrichment
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1360 +/- ##
==========================================
+ Coverage 79.53% 80.66% +1.13%
==========================================
Files 206 206
Lines 11350 11217 -133
==========================================
+ Hits 9027 9048 +21
+ Misses 1779 1622 -157
- Partials 544 547 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/controller/ledger/controller_with_traces_test.go (2)
54-58: 💤 Low valueConsider also covering the
$in[]stringbranch.
accountsFromQueryhas separate handling for$invalues of type[]stringand[]any, but only the[]anyshape is exercised here. Aquery.In("address", []string{...})case would close that gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/ledger/controller_with_traces_test.go` around lines 54 - 58, Add a unit test that calls accountsFromQuery via the existing test harness to exercise the []string-$in branch: add a new test case in the same table (similar to the "in operator with several accounts" entry) using query.In("address", []string{"users:001", "users:002"}) and assert expectedAccounts == []string{"users:001", "users:002"} so the accountsFromQuery code path for []string is covered; locate the table of tests in the controller_with_traces_test.go file and mirror the existing []any test but pass a []string value using query.In.
88-124: 💤 Low valueCoverage gap: pointer variants and the
falsepath are untested.
ResourceQueryFromPaginatedQueryalso handles pointer forms (*InitialPaginatedQuery,*OffsetPaginatedQuery,*ColumnPaginatedQuery) and returns(zero, false)for unrecognized inputs. Only value forms are covered here, so the pointer branches and theok == falsecontract aren't validated.♻️ Example additional cases
{ name: "initial pointer", query: &common.InitialPaginatedQuery[ledger.GetVolumesOptions]{ Options: common.ResourceQuery[ledger.GetVolumesOptions]{Builder: builder}, }, }, // ...plus a nil/unsupported case asserting ok == false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/ledger/controller_with_traces_test.go` around lines 88 - 124, Add tests covering the pointer variants and the false-return path for ResourceQueryFromPaginatedQuery: extend the table in controller_with_traces_test.go to include entries using pointer types (&common.InitialPaginatedQuery, &common.OffsetPaginatedQuery, &common.ColumnPaginatedQuery) that wrap the same ResourceQuery{Builder: builder} and assert the returned rq matches the builder and ok==true, and add an unsupported/nil case (e.g. a nil interface or another type) asserting ok==false and rq equals the zero-value ResourceQuery; reference ResourceQueryFromPaginatedQuery, InitialPaginatedQuery, OffsetPaginatedQuery, ColumnPaginatedQuery and use require.True/require.False and require.Equal to validate results.internal/storage/common/pagination.go (1)
45-58: ⚡ Quick winDefensive handling for typed-nil paginated queries
ResourceQueryFromPaginatedQuerywill panic if thePaginatedQueryinterface contains a typed-nil pointer (e.g.(*InitialPaginatedQuery[...])(nil)), because the pointer type-switch branches dereferencev.Options. Current pagination wiring (storagecommon.Extract/UnmarshalCursorused bygetPaginatedQuery) returns value-form queries, so existingGetVolumesWithBalancespaths won’t trigger this, but guarding the pointer branches keeps the(ResourceQuery[...]{}, false)contract for future callers.Suggested fix
func ResourceQueryFromPaginatedQuery[OptionsType any](q PaginatedQuery[OptionsType]) (ResourceQuery[OptionsType], bool) { switch v := any(q).(type) { case InitialPaginatedQuery[OptionsType]: return v.Options, true case *InitialPaginatedQuery[OptionsType]: + if v == nil { + return ResourceQuery[OptionsType]{}, false + } return v.Options, true case OffsetPaginatedQuery[OptionsType]: return v.Options, true case *OffsetPaginatedQuery[OptionsType]: + if v == nil { + return ResourceQuery[OptionsType]{}, false + } return v.Options, true case ColumnPaginatedQuery[OptionsType]: return v.Options, true case *ColumnPaginatedQuery[OptionsType]: + if v == nil { + return ResourceQuery[OptionsType]{}, false + } return v.Options, true default: return ResourceQuery[OptionsType]{}, false } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/storage/common/pagination.go` around lines 45 - 58, ResourceQueryFromPaginatedQuery currently dereferences v.Options in the pointer type-switch branches and will panic on typed-nil pointers (e.g. (*InitialPaginatedQuery[...])(nil)); update the pointer cases for InitialPaginatedQuery, OffsetPaginatedQuery, and ColumnPaginatedQuery so they first check if v == nil and, if so, return the zero ResourceQuery and false, otherwise return v.Options, true — keep the existing value-type branches unchanged and ensure the function preserves the (ResourceQuery[...]{}, false) contract for nil pointers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/controller/ledger/controller_with_traces_test.go`:
- Around line 54-58: Add a unit test that calls accountsFromQuery via the
existing test harness to exercise the []string-$in branch: add a new test case
in the same table (similar to the "in operator with several accounts" entry)
using query.In("address", []string{"users:001", "users:002"}) and assert
expectedAccounts == []string{"users:001", "users:002"} so the accountsFromQuery
code path for []string is covered; locate the table of tests in the
controller_with_traces_test.go file and mirror the existing []any test but pass
a []string value using query.In.
- Around line 88-124: Add tests covering the pointer variants and the
false-return path for ResourceQueryFromPaginatedQuery: extend the table in
controller_with_traces_test.go to include entries using pointer types
(&common.InitialPaginatedQuery, &common.OffsetPaginatedQuery,
&common.ColumnPaginatedQuery) that wrap the same ResourceQuery{Builder: builder}
and assert the returned rq matches the builder and ok==true, and add an
unsupported/nil case (e.g. a nil interface or another type) asserting ok==false
and rq equals the zero-value ResourceQuery; reference
ResourceQueryFromPaginatedQuery, InitialPaginatedQuery, OffsetPaginatedQuery,
ColumnPaginatedQuery and use require.True/require.False and require.Equal to
validate results.
In `@internal/storage/common/pagination.go`:
- Around line 45-58: ResourceQueryFromPaginatedQuery currently dereferences
v.Options in the pointer type-switch branches and will panic on typed-nil
pointers (e.g. (*InitialPaginatedQuery[...])(nil)); update the pointer cases for
InitialPaginatedQuery, OffsetPaginatedQuery, and ColumnPaginatedQuery so they
first check if v == nil and, if so, return the zero ResourceQuery and false,
otherwise return v.Options, true — keep the existing value-type branches
unchanged and ensure the function preserves the (ResourceQuery[...]{}, false)
contract for nil pointers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d6d93743-8cd7-48fe-8214-6fbcd074adc2
📒 Files selected for processing (3)
internal/controller/ledger/controller_with_traces.gointernal/controller/ledger/controller_with_traces_test.gointernal/storage/common/pagination.go
gfyrag
approved these changes
Jun 3, 2026
gfyrag
approved these changes
Jun 3, 2026
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
2 participants
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.
What
Adds an
accountsOpenTelemetry span attribute to the controller trace spans that operate on accounts, so traces can be filtered/grouped by account in Signoz.Covered operations:
GetAccount,GetAggregatedBalances,GetVolumesWithBalances— accounts extracted from the query builder ($match/$inonaddress/account).SaveAccountMetadata,DeleteAccountMetadata— account taken from the explicitInput.Address.The value is emitted under the same
accountsStringSlicekey already used by the bulk storage operations (storage/ledger/accounts.go), so one query (accounts CONTAINS "users:001") surfaces every span involving an account — single-account or bulk — without type collisions.How
accountsFromQuery(query.Builder) []stringwalks the query expression tree, collecting concrete account addresses from$matchand$infilters (pattern operators like$likeare ignored), then dedupes (slices.Sort+slices.Compact).setAccountsAttribute(ctx, accounts)guards against empty slices and sets theaccountsStringSliceattribute on the current span.common.ResourceQueryFromPaginatedQuery[…]extracts the underlyingResourceQuery(and thus theBuilder) from anyPaginatedQuerystrategy (handles both value and pointer forms).Tests
TestAccountsFromQuery— single/multiple matches,$in, alias dedup+sort, non-string values,$likeexclusion, nil builder.TestResourceQueryFromPaginatedQuery— extraction across Initial / Offset / Column pagination.go build ./internal/...,go vet, and the controller/common test suites pass.Known limitations (intentional, best-effort)
$notnegation:query.Builder.Walkexposes no negation context, so a$not-wrapped address filter would still be tagged. Acceptable for best-effort observability tagging; flagged here for reviewers.GetVolumesWithBalances/GetAggregatedBalancesan address filter can be a segment prefix (e.g.users:) rather than a concrete account, so the attribute reflects the filter, not necessarily exact accounts.🤖 Generated with Claude Code