Skip to content

chore: subs tracing#3897

Open
GAlexIHU wants to merge 1 commit intomainfrom
chore/subs-tracing
Open

chore: subs tracing#3897
GAlexIHU wants to merge 1 commit intomainfrom
chore/subs-tracing

Conversation

@GAlexIHU
Copy link
Copy Markdown
Contributor

@GAlexIHU GAlexIHU commented Feb 24, 2026

Overview

some tracing

Fixes #(issue)

Notes for reviewer

Summary by CodeRabbit

  • Chores
    • Enhanced internal observability and tracing for subscription operations, including creation, updates, synchronization, and addon workflows to improve system monitoring and diagnostics.

@GAlexIHU GAlexIHU requested a review from a team as a code owner February 24, 2026 14:08
@GAlexIHU GAlexIHU added the release-note/ignore Ignore this change when generating release notes label Feb 24, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Feb 24, 2026

📝 Walkthrough

Walkthrough

This PR adds comprehensive OpenTelemetry instrumentation across subscription and workflow services, capturing span attributes and events for subscription operations, syncs, addon workflows, and patch applications without altering control flow or business logic.

Changes

Cohort / File(s) Summary
Subscription Service Instrumentation
openmeter/subscription/service/service.go, openmeter/subscription/service/sync.go
Added span attributes for subscription operations (Create, Update, spec_sync), including namespace, IDs, and operation names. Introduced counters for phase and item changes, plus span events for deletions with context. Post-sync reporting now fetches updated subscription and emits summary telemetry.
Subscription Service Trace Helpers
openmeter/subscription/service/trace.go
New internal helpers for setting span attributes and events. Includes builders for SubscriptionSpec and SubscriptionView attributes with customer IDs, phase/item details, and billable flags, with sorted keys for stable ordering.
Addon Workflow Instrumentation
openmeter/subscription/workflow/service/addon.go
Heavy span attribute instrumentation across addon workflow entry points and processing stages. Added new emitAddonApplyPlanEvents helper for detailed per-addon event emission. Refactored asDiffs to return filtered non-nil diffs. Enhanced sync with before/after addon state tracking.
Subscription Workflow Instrumentation
openmeter/subscription/workflow/service/subscription.go
Systematic span attribute setup in CreateFromPlan, EditRunning, and ChangeToPlan with timing and operation context. Added per-patch event attributes, cadence override logging, and apply-time state tracking during patch application with error context marshaling.
Subscription Workflow Trace Helpers
openmeter/subscription/workflow/service/trace.go
New internal helpers for subscription and addon tracing attributes. Builders for SubscriptionSpec, SubscriptionView, and addon-related attributes with proper nil-span guards and context-aware attribute generation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

release-note/misc

Suggested reviewers

  • tothandras
  • turip
  • chrisgacsal
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'chore: subs tracing' directly reflects the main changes across the PR, which add comprehensive OpenTelemetry tracing instrumentation to subscription services.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/subs-tracing

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
openmeter/subscription/workflow/service/trace.go (1)

32-42: Same nil-phase risk as in service/trace.go.
This deref can panic if a phase entry is nil; consider the same guard here.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openmeter/subscription/workflow/service/trace.go` around lines 32 - 42, The
loop in addSubscriptionSpecAttrs iterates spec.Phases and dereferences
phase.ItemsByKey which can panic if a phase is nil; update the for k, phase :=
range spec.Phases loop to guard against nil (e.g., if phase == nil { continue })
before accessing phase.ItemsByKey and before appending to
phaseKeys/itemVersions/itemKeySet so nil phase entries are skipped safely;
ensure phaseKeys, itemKeySet and itemVersions logic remains unchanged for
non-nil phases.
🧹 Nitpick comments (1)
openmeter/subscription/workflow/service/subscription.go (1)

226-239: Avoid eager JSON marshaling on the happy path.
specBeforeApplyJSON is marshaled unconditionally but only used on error. Deferring this to the error handler reduces overhead for successful edits.

♻️ Suggested tweak
-	// TODO: remove after issue is fixed
-	specBeforeApplyJSON, _ := json.Marshal(spec)
 	logApplyErr := func(mErr error) {
+		// TODO: remove after issue is fixed
+		specBeforeApplyJSON, _ := json.Marshal(spec)
 		customizationsJSON, err := json.Marshal(customizations)
 		if err != nil {
 			s.Logger.DebugContext(ctx, "failed to marshal customizations for error logging", "error", err)
 		}

As per coding guidelines: "Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openmeter/subscription/workflow/service/subscription.go` around lines 226 -
239, Currently specBeforeApplyJSON is marshaled eagerly before defining
logApplyErr, causing unnecessary work on the happy path; move the
json.Marshal(spec) call into the logApplyErr function so the spec is only
marshaled when mErr is non-nil, and handle/mask marshal errors similarly to how
customizationsJSON is handled (log a debug message on marshal failure and avoid
panics), updating the s.Logger.DebugContext call to reference the locally
marshaled spec JSON instead of specBeforeApplyJSON; keep the rest of logApplyErr
behavior (customizations marshal, editTime, apply_error) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@openmeter/subscription/service/trace.go`:
- Around line 31-41: The loop in addSpecAttrs reads phase.ItemsByKey but
spec.Phases contains pointers, so if a phase is nil this will panic; update the
loop over spec.Phases in addSpecAttrs to check if phase == nil and skip/continue
that entry before accessing phase.ItemsByKey (i.e., guard the use of
phase.ItemsByKey and phase-related logic with a nil check) to prevent trace
crashes on malformed/partial specs.

In `@openmeter/subscription/workflow/service/addon.go`:
- Around line 418-423: Filtering out nil entries from the diffs slice breaks the
expected 1:1 alignment with the addons list; instead of lo.Filtering nils,
detect nil returns from GetDiffableFromAddon and return an error (or wrap
context) so callers keep positional mapping. Locate the code that builds the
diffs slice (where GetDiffableFromAddon is called and addondiff.Diffable entries
are collected), replace the silent nil-drop with an explicit nil-check that
returns an error mentioning the addon identifier and the function
GetDiffableFromAddon, and remove the lo.Filter step (or keep it only after the
nil-check) so callers relying on equal lengths/positions are preserved.

---

Duplicate comments:
In `@openmeter/subscription/workflow/service/trace.go`:
- Around line 32-42: The loop in addSubscriptionSpecAttrs iterates spec.Phases
and dereferences phase.ItemsByKey which can panic if a phase is nil; update the
for k, phase := range spec.Phases loop to guard against nil (e.g., if phase ==
nil { continue }) before accessing phase.ItemsByKey and before appending to
phaseKeys/itemVersions/itemKeySet so nil phase entries are skipped safely;
ensure phaseKeys, itemKeySet and itemVersions logic remains unchanged for
non-nil phases.

---

Nitpick comments:
In `@openmeter/subscription/workflow/service/subscription.go`:
- Around line 226-239: Currently specBeforeApplyJSON is marshaled eagerly before
defining logApplyErr, causing unnecessary work on the happy path; move the
json.Marshal(spec) call into the logApplyErr function so the spec is only
marshaled when mErr is non-nil, and handle/mask marshal errors similarly to how
customizationsJSON is handled (log a debug message on marshal failure and avoid
panics), updating the s.Logger.DebugContext call to reference the locally
marshaled spec JSON instead of specBeforeApplyJSON; keep the rest of logApplyErr
behavior (customizations marshal, editTime, apply_error) intact.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b128019 and 8233a99.

📒 Files selected for processing (6)
  • openmeter/subscription/service/service.go
  • openmeter/subscription/service/sync.go
  • openmeter/subscription/service/trace.go
  • openmeter/subscription/workflow/service/addon.go
  • openmeter/subscription/workflow/service/subscription.go
  • openmeter/subscription/workflow/service/trace.go

Comment on lines +31 to +41
func addSpecAttrs(attrs []attribute.KeyValue, prefix string, spec subscription.SubscriptionSpec) []attribute.KeyValue {
phaseKeys := make([]string, 0, len(spec.Phases))
itemKeySet := make(map[string]struct{})
itemVersions := 0
for k, phase := range spec.Phases {
phaseKeys = append(phaseKeys, k)
for ik, items := range phase.ItemsByKey {
itemKeySet[ik] = struct{}{}
itemVersions += len(items)
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Guard against nil phase entries to avoid panics.
SubscriptionSpec.Phases stores pointers; if a phase is nil (e.g., during patching/invalid input), phase.ItemsByKey will panic before validation kicks in. A small nil check keeps tracing from crashing the operation.

🛠️ Suggested guard
 for k, phase := range spec.Phases {
+	if phase == nil {
+		continue
+	}
 	phaseKeys = append(phaseKeys, k)
 	for ik, items := range phase.ItemsByKey {
 		itemKeySet[ik] = struct{}{}
 		itemVersions += len(items)
 	}
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func addSpecAttrs(attrs []attribute.KeyValue, prefix string, spec subscription.SubscriptionSpec) []attribute.KeyValue {
phaseKeys := make([]string, 0, len(spec.Phases))
itemKeySet := make(map[string]struct{})
itemVersions := 0
for k, phase := range spec.Phases {
phaseKeys = append(phaseKeys, k)
for ik, items := range phase.ItemsByKey {
itemKeySet[ik] = struct{}{}
itemVersions += len(items)
}
}
func addSpecAttrs(attrs []attribute.KeyValue, prefix string, spec subscription.SubscriptionSpec) []attribute.KeyValue {
phaseKeys := make([]string, 0, len(spec.Phases))
itemKeySet := make(map[string]struct{})
itemVersions := 0
for k, phase := range spec.Phases {
if phase == nil {
continue
}
phaseKeys = append(phaseKeys, k)
for ik, items := range phase.ItemsByKey {
itemKeySet[ik] = struct{}{}
itemVersions += len(items)
}
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openmeter/subscription/service/trace.go` around lines 31 - 41, The loop in
addSpecAttrs reads phase.ItemsByKey but spec.Phases contains pointers, so if a
phase is nil this will panic; update the loop over spec.Phases in addSpecAttrs
to check if phase == nil and skip/continue that entry before accessing
phase.ItemsByKey (i.e., guard the use of phase.ItemsByKey and phase-related
logic with a nil check) to prevent trace crashes on malformed/partial specs.

Comment on lines +418 to 423
filtered := lo.Filter(diffs, func(d addondiff.Diffable, _ int) bool {
return d != nil
}), nil
})

return filtered, nil
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Filtering nil diffables changes semantics — watch 1:1 assumptions.
If GetDiffableFromAddon can return nil, dropping entries here means any caller that expects alignment with the addon list (e.g., length checks or positional mapping) can now fail or undercount. Either treat nil as an error or update callers to handle a reduced slice.

🛠️ Option: treat nil as an error to preserve 1:1 mapping
-	filtered := lo.Filter(diffs, func(d addondiff.Diffable, _ int) bool {
-		return d != nil
-	})
-
-	return filtered, nil
+	for i, d := range diffs {
+		if d == nil {
+			return nil, fmt.Errorf("nil diffable for addon at index %d", i)
+		}
+	}
+
+	return diffs, nil
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
filtered := lo.Filter(diffs, func(d addondiff.Diffable, _ int) bool {
return d != nil
}), nil
})
return filtered, nil
}
for i, d := range diffs {
if d == nil {
return nil, fmt.Errorf("nil diffable for addon at index %d", i)
}
}
return diffs, nil
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openmeter/subscription/workflow/service/addon.go` around lines 418 - 423,
Filtering out nil entries from the diffs slice breaks the expected 1:1 alignment
with the addons list; instead of lo.Filtering nils, detect nil returns from
GetDiffableFromAddon and return an error (or wrap context) so callers keep
positional mapping. Locate the code that builds the diffs slice (where
GetDiffableFromAddon is called and addondiff.Diffable entries are collected),
replace the silent nil-drop with an explicit nil-check that returns an error
mentioning the addon identifier and the function GetDiffableFromAddon, and
remove the lo.Filter step (or keep it only after the nil-check) so callers
relying on equal lengths/positions are preserved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note/ignore Ignore this change when generating release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants