fix(balanceworker): subject lookup#4356
Conversation
📝 WalkthroughWalkthroughThis PR makes the subject resolution flow more resilient by treating missing subjects defensively. When ChangesSubject Resolution Error Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openmeter/entitlement/balanceworker/subject_customer.go (2)
48-56: ⚖️ Poor tradeoffConsider whether this fallback belongs in the subject service layer.
Right now, each caller of
subjectService.GetByKeywould need to implement this same fallback logic if they want to handle missing subjects gracefully. Moving this behavior into the subject service itself (or providing aGetByKeyOrSyntheticmethod) would centralize the logic and ensure consistency across the codebase.🤖 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 `@openmeter/entitlement/balanceworker/subject_customer.go` around lines 48 - 56, The missing-subject fallback currently implemented at the call site (checking models.IsGenericNotFoundError and returning a synthetic *subject.Subject) should be moved into the subject service to centralize behavior; either modify subjectService.GetByKey to return a synthetic subject when not found or add a new helper like subjectService.GetByKeyOrSynthetic that encapsulates the logic creating the synthetic subject (Namespace, Key) and returns it instead of surfacing the not-found error, and update callers (including the logic around *cus) to call the new/updated service method so they no longer duplicate the models.IsGenericNotFoundError check.
48-56: ⚡ Quick winConsider adding observability when falling back to synthetic subject.
The silent fallback makes it harder to monitor how often this scenario occurs or debug issues later. Adding a log or metric when creating the synthetic subject would help with visibility.
📊 Suggestion: Add logging for observability
if err != nil { // Subjects are no longer persisted as their own entity, so a missing row is // expected. Fall back to a synthetic subject carrying just the key so the // snapshot event still reports the usage-attribution key downstream. if models.IsGenericNotFoundError(err) { + // Log this fallback for monitoring/debugging + // (Adjust logger as appropriate for your codebase) + logger.Debug(ctx, "subject not found, using synthetic subject", + "namespace", namespace, + "key", subjKey, + ) return *cus, &subject.Subject{ Namespace: namespace, Key: subjKey, }, nil }🤖 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 `@openmeter/entitlement/balanceworker/subject_customer.go` around lines 48 - 56, When models.IsGenericNotFoundError(err) triggers and you return the synthetic subject, add an observability call (log and/or metric) inside that branch to record the fallback occurrence; include identifying fields like namespace, subjKey and the customer identifier from cus (e.g., cus.ID) and the error string to aid debugging. Locate the branch around models.IsGenericNotFoundError in subject_customer.go and emit the log/metric before returning the synthetic subject (use the package's existing logger or metrics client). Ensure the message is concise and structured (event name like "synthetic_subject_fallback") and avoid exposing sensitive data.
🤖 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 `@openmeter/entitlement/balanceworker/subject_customer.go`:
- Around line 48-56: The missing-subject fallback currently implemented at the
call site (checking models.IsGenericNotFoundError and returning a synthetic
*subject.Subject) should be moved into the subject service to centralize
behavior; either modify subjectService.GetByKey to return a synthetic subject
when not found or add a new helper like subjectService.GetByKeyOrSynthetic that
encapsulates the logic creating the synthetic subject (Namespace, Key) and
returns it instead of surfacing the not-found error, and update callers
(including the logic around *cus) to call the new/updated service method so they
no longer duplicate the models.IsGenericNotFoundError check.
- Around line 48-56: When models.IsGenericNotFoundError(err) triggers and you
return the synthetic subject, add an observability call (log and/or metric)
inside that branch to record the fallback occurrence; include identifying fields
like namespace, subjKey and the customer identifier from cus (e.g., cus.ID) and
the error string to aid debugging. Locate the branch around
models.IsGenericNotFoundError in subject_customer.go and emit the log/metric
before returning the synthetic subject (use the package's existing logger or
metrics client). Ensure the message is concise and structured (event name like
"synthetic_subject_fallback") and avoid exposing sensitive data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6468276e-0f95-4749-a694-a5bca8598e24
📒 Files selected for processing (1)
openmeter/entitlement/balanceworker/subject_customer.go
Summary by CodeRabbit