feat(metrics): Add public API to collect count, distribution and gauge #6957
Conversation
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
philprime
requested review from
itaybre,
noahsmartin and
philipphofmann
as code owners
7 tasks
Contributor
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6957 +/- ##
=============================================
+ Coverage 84.909% 85.050% +0.141%
=============================================
Files 465 468 +3
Lines 28050 28222 +172
Branches 12385 12514 +129
=============================================
+ Hits 23817 24003 +186
+ Misses 4189 4176 -13
+ Partials 44 43 -1
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
philprime
changed the base branch from
philprime/metrics-bootstrap
to
philprime/metrics
philprime
force-pushed
the
philprime/metrics-public-api
branch
from
7158dfc to
87379ed
Compare
7 tasks
philprime
force-pushed
the
philprime/metrics-public-api
branch
from
4c30f13 to
02c3573
Compare
philprime
changed the base branch from
philprime/metrics
to
philprime/metrics-envelope
philprime
force-pushed
the
philprime/metrics-public-api
branch
from
02c3573 to
1ab754c
Compare
Member
philipphofmann
left a comment
philipphofmann
left a comment
There was a problem hiding this comment.
Deep Review Summary
I've completed a thorough review comparing this implementation against the official metrics spec and the JavaScript SDK.
🤖 This review was generated with Claude Code to ensure comprehensive spec compliance checking and cross-SDK consistency analysis.
🎯 Overall Assessment: High Quality Implementation (85% Spec Compliant)
This is an excellent, well-architected implementation with several Swift-native improvements over the JavaScript SDK. The type-safe value system, dependency injection pattern, and attribute handling are exemplary.
📊 Priority Breakdown
🔴 HIGH Priority (2 issues in this PR) - Must fix before merge
🟡 MEDIUM Priority (2 issues in this PR) - Should address
🟢 LOW Priority + Praise (3 items) - Nice to have + excellent work
Note: Some issues relate to files from previous PRs (SentryMetricValue, SentryAttributeValue, SentryMetricsBatcher) which I'll comment on separately or in those PRs.
✨ Outstanding Work
Special recognition for:
- Clean protocol-based API design
- Dependency injection pattern for testability
- Comprehensive test coverage (unit + E2E)
- Excellent sample code
- Type-safe unit enum with generic escape hatch
📋 Spec Compliance: 10/12 Requirements
The main deviations are:
- ❌ Missing counter default value (easy fix - this PR)
⚠️ Unsigned counter vs signed (previous PR - SentryMetricValue)- ✅ Typed unit enum (improvement over spec's free-form strings)
🚀 Recommendation
Fix the 2 HIGH priority issues in this PR, then this is ready to merge. The other issues can be addressed in the related PRs or follow-ups.
Great work on this feature! 🎉
🔄 How to Reproduce This Review
Click to expand: Instructions for re-running this AI-assisted review
Prerequisites
- Claude Code CLI installed and authenticated
- Access to the repository
Steps to Reproduce
-
Navigate to the repository:
cd /path/to/sentry-cocoa -
Open Claude Code in your terminal:
claude
-
Use this prompt:
Please do a very deep review of this PR https://github.com/getsentry/sentry-cocoa/pull/6957. Please don't make any comments to the PR, just give me some comments based on the LOGAF scale. Please also double check very closely if the metrics API aligns with what is specified here https://develop.sentry.dev/sdk/telemetry/metrics/ and also compare it with the metrics implementation of the JS sentry SDK https://github.com/getsentry/sentry-javascript. -
After receiving the analysis, ask Claude to create the GitHub review:
Now please add them to a new GH PR review for this PR and not submit the review please. So I can double check the comments on GH and please use the logaf scale with `h`, `m`, `l` prefixes for the comments.
What Claude Does
- Fetches PR details using
gh pr view 6957 - Retrieves the official spec from https://develop.sentry.dev/sdk/telemetry/metrics/
- Analyzes the JavaScript SDK implementation for comparison
- Reads all modified files in the PR
- Compares implementation against spec requirements
- Generates LOGAF-scaled comments (h=high, m=medium, l=low priority)
- Creates a pending GitHub review via the GitHub API
Review Coverage
The AI review checks:
- ✅ Spec compliance (all required methods, parameters, defaults)
- ✅ Cross-SDK consistency (comparing with JavaScript SDK)
- ✅ API ergonomics (Swift-native patterns)
- ✅ Type safety and error handling
- ✅ Documentation completeness
- ✅ Test coverage
- ✅ Edge cases and potential bugs
Benefits of AI-Assisted Review
- Comprehensive: Checks every spec requirement systematically
- Consistent: Uses same criteria across all reviews
- Cross-SDK awareness: Compares with other language implementations
- ✅ Fast: Analyzes thousands of lines in minutes
- Learning: Discovers patterns and suggests improvements
- Documented: Provides reasoning for each comment
Limitations
- May miss context-specific business logic issues
- Cannot assess UX/design decisions without user input
- Line number references may break if PR is updated
- Best used as a complement to human review, not replacement
Follow-up Reviews
To review subsequent commits:
# Get the new commit SHA
git log origin/philprime/metrics-public-api -1 --format=%H
# Ask Claude to review changes since last review
claude
# Then: "Please review the changes in commit <SHA> against the same spec and JS SDK"
Sources/Swift/Integrations/Metrics/SentryMetricsApiProtocol.swift
Outdated
Show resolved
Hide resolved
Expands the headerdoc for the metrics property to include: - Description of what Sentry Metrics does and its capabilities - Documentation of the three metric types (count, gauge, distribution) - Code examples showing typical usage patterns - Requirements section referencing enableMetrics option - Beta status note - Important callout explaining Swift-only availability - Link to full documentation at docs.sentry.io/platforms/apple/metrics/
- Add customer-friendly warning logs when metrics are recorded without SDK started or metrics enabled - Use SentryCurrentDateProvider instead of Date() for testability - Set traceId at metric creation time instead of deferring to batcher to prevent empty traceIds if enrichment fails - Add comprehensive comments explaining the intentional traceId logic duplication with BatcherScope - Update test mock to include dateProvider dependency
…back - Fix count value description: UInt is always non-negative, add default note - Add default value of 1 for count per metrics spec - Improve unit parameter docs to reference SentryMetricsUnit enum cases - Fix attributes docs: list correct types (String, Bool, Int, Double + arrays) - Document that mixed arrays and unsupported types are converted to strings - Add comment explaining protocol extension pattern for default parameters
- Update docs to clarify units are for telemetry data (Metrics, Spans, Logs) - Add Equatable to enum declaration, remove manual implementation - Add comment explaining why RawRepresentable is implemented manually (to support custom units via .generic(String) associated value)
- Add XCTAssertNil assertions to count/distribution/gauge disabled tests - Clarify in comments that when integration is nil, there's no mock to verify invocations on - assertion confirms test precondition - Remove commented empty line before MARK comment
philipphofmann
approved these changes
Jan 15, 2026
Member
There was a problem hiding this comment.
LGTM, thanks 🚀
Post edit: LGTM, after resolving the SentryUnit discussion above #6957 (comment)
…eMetric - Enable metrics by default (options.experimental.enableMetrics = true) - Rename internal recordMetric to captureMetric to align with SDK terminology - Update changelog and documentation to reflect the new default - Fix warning message to reference correct option path
philipphofmann
approved these changes
Jan 15, 2026
philprime
removed
the
dontmerge
Contributor
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Metrics
Other
Bug Fixes 🐛
Build / dependencies / internal 🔧Deps
Other
Other
🤖 This preview updates automatically when you update the PR. |
Enhanced the documentation for the metrics feature, explaining that while metrics are enabled by default, they require manual API calls to function and do not automatically capture metrics or affect application behavior. Updated instructions for disabling metrics.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
- Consolidated entries in CHANGELOG for metrics integration and API. - Clarified documentation in SentrySDK regarding the default behavior of metrics and the necessity of manual API calls for functionality.
- Removed unnecessary blank lines throughout the test file to improve readability and maintainability. - Ensured consistent formatting in the test cases for better clarity.
- Added "Metrics" to the expected integrations in SentryClientTests and SentrySDKTests. - Updated assertions in SentryEnabledFeaturesBuilderTests to include "metrics" in the expected features. - Adjusted SentrySwiftIntegrationInstallerTests to set enableMetrics to false in options. - Enhanced the xcodebuild script to remove existing result bundles before running tests.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR is part of a merge-chain and should be merged one-by-one into
mainas soon as all of them are ready to be merged:This PR must also wait for #7077 to be merged
📜 Description
This pull request introduces a new metrics API to the codebase, allowing clients to record custom metrics such as counts, distributions, and gauges. The API is exposed via the
SentrySDK.metricsproperty and is designed for future extensibility, though the current implementation is a no-op.This PR also adds an example UI to manually collect metrics in iOS-Swift.
💡 Motivation and Context
Closes #6949
💚 How did you test it?
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.TODO
AttributableandAttributeValueto support type-safe bool, string, int, float, double literal values and variablesAddAttributeArrayValueto support type-safe arrays without mixed types