Fix CallHomeReporter payload validation failure #3727
Conversation
…y payload The remote telemetry server requires 'mode' and 'median' fields in summary metrics for validation. These were removed in commit 6a64347 as part of memory optimization work. This adds placeholder values (median: 0, mode: nil) to satisfy the remote schema validation without reverting the memory-efficient running-tally approach for computing statistics.
…tests This dependency will be used to validate CallHomeReporter payloads against the remote server's JSON schema, guarding against validation regressions.
This schema is a copy of the one used by the remote telemetry server to validate incoming CallHomeReporter payloads. If either schema changes, both must be kept in sync to prevent validation failures.
This test validates that StackTelemetry report data conforms to the remote server's JSON schema, preventing regressions where payload changes break remote validation. The test uses @external_resource to track the schema file, ensuring the test recompiles when the schema changes.
📝 WalkthroughWalkthroughDefault Summary metric payloads now include Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🪛 markdownlint-cli2 (0.18.1).changeset/pink-pumpkins-switch.md5-5: First line in a file should be a top-level heading (MD041, first-line-heading, first-line-h1) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (3)
✏️ Tip: You can disable this entire section by setting Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3727 +/- ##
==========================================
- Coverage 84.01% 82.94% -1.08%
==========================================
Files 44 35 -9
Lines 2834 2380 -454
Branches 534 529 -5
==========================================
- Hits 2381 1974 -407
+ Misses 451 404 -47
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/electric-telemetry/lib/electric/telemetry/measurement.ex`:
- Around line 153-159: Update the moduledoc in the Measurement module to reflect
the new summary shape that now includes median and mode placeholders: modify the
documentation in
packages/electric-telemetry/lib/electric/telemetry/measurement.ex (the moduledoc
for the Measurement module) to list min, max, mean, median, and mode in the
summary structure and update any examples and descriptive text to mention that
median defaults to 0 and mode to nil (or their intended types/semantics); ensure
the documented shape matches the actual map returned in the code snippets around
the summary construction (the map containing min, max, mean, median, mode).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/electric-telemetry/mix.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
packages/electric-telemetry/lib/electric/telemetry/call_home_reporter.expackages/electric-telemetry/lib/electric/telemetry/measurement.expackages/electric-telemetry/mix.exspackages/electric-telemetry/test/electric/telemetry/call_home_reporter_test.exspackages/electric-telemetry/test/electric/telemetry/measurement_test.exspackages/electric-telemetry/test/support/call_home_remote_schema.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test packages/typescript-client w/ sync-service
- GitHub Check: Test packages/experimental w/ sync-service
- GitHub Check: Test packages/react-hooks w/ sync-service
- GitHub Check: Test packages/y-electric w/ sync-service
- GitHub Check: Test packages/start w/ sync-service
- GitHub Check: Run Lux integration tests
🔇 Additional comments (6)
packages/electric-telemetry/lib/electric/telemetry/call_home_reporter.ex (1)
204-207: Default summary payload now aligns with schema.Line 206 adds median/mode placeholders to the default summary map, keeping payloads consistent with the updated summary shape. Looks good.
packages/electric-telemetry/test/electric/telemetry/measurement_test.exs (2)
285-479: Summary tests aligned with placeholder median/mode fields.The added assertions (Line 293-478) consistently cover the new
medianandmodeplaceholders. This keeps the summary test suite aligned with the updated payload shape.
561-565: Multi-metric interaction test updated correctly.The additional median/mode assertions (Line 564-565) align with the new summary structure. Looks good.
packages/electric-telemetry/test/electric/telemetry/call_home_reporter_test.exs (1)
4-14: The current validation is correct; no changes needed.The schema (call_home_remote_schema.json) is designed for the data payload contents, not the full report envelope. Its root properties (
electric_version,environment,resources,usage) and required fields match the structure withinreport["data"]. Validatingreport["data"]against@remote_schemaat line 126 is the intended behavior.Likely an incorrect or invalid review comment.
packages/electric-telemetry/mix.exs (1)
39-41: No action needed. The JSON schema uses draft-06, which is supported byex_json_schema "~> 0.10".packages/electric-telemetry/test/support/call_home_remote_schema.json (1)
1-177: LGTM! Schema correctly captures the remote validation requirements.The schema is well-structured and aligns with the PR objectives:
- The
Summarydefinition (lines 109-138) correctly requiresmedianandmodefields- The
modefield usesanyOf: [number, null](lines 126-133) which properly supports themode: nilplaceholder value- Using
additionalProperties: falseon nested definitions ensures strict validation that mirrors the remote server's behaviorThis schema will effectively catch payload regressions in tests before they reach the remote telemetry server.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.changeset/pink-pumpkins-switch.md:
- Line 5: Update the changeset description to reflect that the restored median
and mode fields apply to all Summary metrics in CallHomeReporter (not just
wal_size); edit the message in .changeset/pink-pumpkins-switch.md so it states
something like: "Bring back previously removed median and mode fields to Summary
metrics in CallHomeReporter’s default payload (applies to run_queue_total,
run_queue_cpu, run_queue_io, wal_size); their absence caused the remote
collector to reject reports." Reference CallHomeReporter (call_home_reporter.ex)
and the Summary metric defaults so the scope is clear.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.changeset/pink-pumpkins-switch.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
.changeset/pink-pumpkins-switch.md
5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Document that summaries now include median and mode placeholder fields (defaulting to 0 and nil respectively) for compatibility with downstream consumers that expect these fields in the summary structure.
alco
force-pushed
the
alco/call-home-reporter-payload-fix
branch
from
70be42c to
1fbfa3f
Compare
Summary
modeandmedianfields to CallHomeReporter summary metrics payloadProblem
PR #3422 optimized CallHomeReporter memory usage and removed
modeandmedianfrom summary statistics as a side-effect since calculating those would have interfered with the optimization effort and they weren't used in reporting analytics anyway. However, the remote telemetry server requires these fields for validation, causing all reports to be rejected silently.Solution
Add placeholder values (
median: 0,mode: nil) to satisfy the remote schema validation without reverting the memory-efficient running-tally approach. I have chosen to make these changes instead of changing the remote server's validation because the latter is not actively maintained and was last deployed 7 months ago. I deemed making changes to it too risky.Regression Prevention
Added a test that validates the payload against a copy of the remote server's JSON schema. If the payload structure changes in a way that would break remote validation, this test will fail.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.