fix: disable http req duration metric by default#281
fix: disable http req duration metric by default#281SoulPancake wants to merge 6 commits intomainfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis change disables the fga-client.http_request.duration metric by default in the telemetry configuration. The metric can still be enabled via explicit configuration. Guards are added to safely handle nil metric configurations, and tests verify the default-disabled behavior. Changes
Possibly related PRs
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the tone of the review comments and chat replies.Configure the |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (33.70%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #281 +/- ##
==========================================
+ Coverage 33.63% 33.70% +0.06%
==========================================
Files 115 115
Lines 9831 9826 -5
==========================================
+ Hits 3307 3312 +5
+ Misses 6260 6244 -16
- Partials 264 270 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Related Documentation Checked 8 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
telemetry/metrics_test.go (1)
292-330: Strengthen disabled-path assertions to verify no instrument creation.The subtests check returned histogram is
nil, which is good. Consider also asserting that no histogram was created in the meter to catch future regressions in side effects.♻️ Suggested test hardening
func TestHTTPRequestDuration_DisabledByDefault(t *testing.T) { - mockMeter := &MockMeter{ - counters: make(map[string]metric.Int64Counter), - histograms: make(map[string]metric.Float64Histogram), - } - t.Run("nil configuration", func(t *testing.T) { + mockMeter := &MockMeter{ + counters: make(map[string]metric.Int64Counter), + histograms: make(map[string]metric.Float64Histogram), + } metrics := &Metrics{ Meter: mockMeter, Histograms: make(map[string]metric.Float64Histogram), Configuration: nil, } @@ if histogram != nil { t.Fatalf("Expected nil histogram when metric is disabled (nil config), but got non-nil") } + if len(mockMeter.histograms) != 0 { + t.Fatalf("Expected no histogram instruments to be created when disabled") + } }) t.Run("nil metric config (disabled by default)", func(t *testing.T) { + mockMeter := &MockMeter{ + counters: make(map[string]metric.Int64Counter), + histograms: make(map[string]metric.Float64Histogram), + } metrics := &Metrics{ Meter: mockMeter, Histograms: make(map[string]metric.Float64Histogram), // METRIC_HISTOGRAM_HTTP_REQUEST_DURATION is nil — this is the default. Configuration: &MetricsConfiguration{ @@ if histogram != nil { t.Fatalf("Expected nil histogram when metric is disabled, but got non-nil") } + if len(mockMeter.histograms) != 0 { + t.Fatalf("Expected no histogram instruments to be created when disabled") + } }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@telemetry/metrics_test.go` around lines 292 - 330, The test only asserts the returned histogram is nil but should also assert that no histogram instrument was created on the meter; update TestHTTPRequestDuration_DisabledByDefault (inside both subtests) to inspect the MockMeter (mockMeter.histograms) after calling Metrics.HTTPRequestDuration and assert it has no entries (or that the expected histogram key is not present) to catch side-effect regressions; place these assertions immediately after the existing nil-checks in the two subtests and use the MockMeter.histograms map to verify no instrument creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@telemetry/metrics_test.go`:
- Around line 292-330: The test only asserts the returned histogram is nil but
should also assert that no histogram instrument was created on the meter; update
TestHTTPRequestDuration_DisabledByDefault (inside both subtests) to inspect the
MockMeter (mockMeter.histograms) after calling Metrics.HTTPRequestDuration and
assert it has no entries (or that the expected histogram key is not present) to
catch side-effect regressions; place these assertions immediately after the
existing nil-checks in the two subtests and use the MockMeter.histograms map to
verify no instrument creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 144cbeb5-89a8-4996-b9b6-1281bb4c1c04
📒 Files selected for processing (5)
CHANGELOG.mdtelemetry/configuration.gotelemetry/configuration_test.gotelemetry/metrics.gotelemetry/metrics_test.go
There was a problem hiding this comment.
Pull request overview
This PR makes the fga-client.http_request.duration OpenTelemetry histogram opt-in by disabling it in the default telemetry configuration, and ensuring the SDK skips recording when it’s not enabled.
Changes:
- Disable
METRIC_HISTOGRAM_HTTP_REQUEST_DURATIONinDefaultTelemetryConfiguration()by defaulting it tonil. - Skip creating/recording the HTTP request duration histogram when the metric isn’t enabled in configuration.
- Add/adjust tests to validate enabled behavior and “disabled by default” behavior, and document the change in the changelog.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| telemetry/metrics.go | Skips recording/creating the HTTP request duration histogram unless explicitly enabled. |
| telemetry/metrics_test.go | Updates existing test to explicitly enable the metric; adds tests verifying disabled-by-default behavior. |
| telemetry/configuration.go | Sets the HTTP request duration metric config to nil in defaults (opt-in). |
| telemetry/configuration_test.go | Asserts the HTTP request duration metric is nil (disabled) by default. |
| CHANGELOG.md | Notes behavior change under Unreleased. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (m *Metrics) HTTPRequestDuration(value float64, attrs map[*Attribute]string) (metric.Float64Histogram, error) { | ||
| // If the metric is not configured (nil), skip recording entirely — it is disabled by default. | ||
| if m.Configuration == nil || m.Configuration.METRIC_HISTOGRAM_HTTP_REQUEST_DURATION == nil { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
HTTPRequestDuration now returns (nil, nil) when the metric is disabled. Since the return type is an interface, callers may not expect a nil histogram and could dereference it without checking. Consider returning a no-op histogram (or at least a consistent non-nil histogram while skipping Record) to keep the method contract consistent/safe, or ensure this nil-return convention is applied consistently across other metric methods as well.
|
|
||
| ## [Unreleased](https://github.com/openfga/go-sdk/compare/v0.7.5...HEAD) | ||
|
|
||
| - fix: The `fga-client.http_request.duration` metric is now disabled by default. Users can enable it via telemetry configuration if needed. |
There was a problem hiding this comment.
While it is a correction in behaviour, I think we should we call this out as a breaking change?
|
|
||
| func (m *Metrics) HTTPRequestDuration(value float64, attrs map[*Attribute]string) (metric.Float64Histogram, error) { | ||
| // If the metric is not configured (nil), skip recording entirely — it is disabled by default. | ||
| if m.Configuration == nil || m.Configuration.METRIC_HISTOGRAM_HTTP_REQUEST_DURATION == nil { |
There was a problem hiding this comment.
I find it weird that we don't have this check already as to disable a metric you have to set nil already? Do we not actually support disabling metrics given no other codepath has this?
There was a problem hiding this comment.
Yep I think there are no other opt-in metrics right now
So this is a first
And the other ones are like enabled by default so this check never triggers for them ( they don't have the check there )
Description
Disables the duration metric by default ( makes it opt-in )
openfga/dotnet-sdk#173 (review)
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit
fga-client.http_request.durationmetric is now disabled by default to improve performance. Users can enable it via telemetry configuration if monitoring is required.