feat: report a per http call metric#173
Conversation
|
Related Documentation Checked 8 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughIntroduces HTTP request duration telemetry tracking across the OpenFga SDK. A new Changes
Sequence DiagramsequenceDiagram
actor Client
participant ApiClient
participant BaseClient
participant HttpClient
participant Metrics
participant Histogram
Client->>ApiClient: Make API Request
ApiClient->>ApiClient: Create/Initialize Metrics
ApiClient->>BaseClient: new BaseClient(..., metrics)
Client->>ApiClient: SendRequest()
ApiClient->>BaseClient: SendRequestAsync<TRes>()
BaseClient->>BaseClient: Start Stopwatch
BaseClient->>HttpClient: SendAsync(request)
HttpClient-->>BaseClient: HttpResponseMessage
BaseClient->>BaseClient: EnsureSuccessStatusCode()
BaseClient->>BaseClient: Deserialize Response
alt Metrics Enabled
BaseClient->>Metrics: BuildForHttpRequest()
Metrics->>Metrics: Compute Attributes
Metrics->>Histogram: RecordHttpRequestDuration()
Histogram->>Histogram: Record Duration in Histogram
end
BaseClient-->>ApiClient: TRes response
ApiClient-->>Client: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@OpenTelemetry.md`:
- Line 16: Fix the typo in the metric description for
`fga-client.query.duration`: change "internally process nd evaluate the request"
to "internally process and evaluate the request" so the description reads
correctly.
- Line 18: The code span in OpenTelemetry.md for the metric name is written with
a leading space ("` fga-client.credentials.request`") which breaks rendering;
edit that table cell to remove the leading space so it reads
"`fga-client.credentials.request`" (update the exact token
fga-client.credentials.request in the table row to ensure correct code span
formatting).
In `@src/OpenFga.Sdk/ApiClient/BaseClient.cs`:
- Around line 84-91: Update SendRequestAsync to accept an int retryCount
parameter (default 0 for backward compatibility) and propagate that value into
the metrics call instead of the hardcoded 0; specifically, change the
SendRequestAsync signature to include retryCount, update the call site in
ApiClient.Retry to pass the current attempt count, and replace the literal 0 in
the _metrics.BuildForHttpRequest(...) invocation with the retryCount value so
http.request.resend_count reflects the actual attempt number.
🧹 Nitpick comments (2)
src/OpenFga.Sdk/Configuration/TelemetryConfig.cs (1)
73-78: Avoid sharing the same attribute set across default metrics.All MetricConfig entries reference the same
HashSetinstance, so mutating one metric’sAttributesmutates the others. Consider cloning per metric to avoid cross-contamination.♻️ Suggested update
- return new Dictionary<string, MetricConfig> { - { TelemetryMeter.TokenExchangeCount, new MetricConfig { Attributes = defaultAttributes } }, - { TelemetryMeter.RequestDuration, new MetricConfig { Attributes = defaultAttributes } }, - { TelemetryMeter.QueryDuration, new MetricConfig { Attributes = defaultAttributes } }, - { TelemetryMeter.HttpRequestDuration, new MetricConfig { Attributes = defaultAttributes } }, - // { TelemetryMeters.RequestCount, new MetricConfig { Attributes = defaultAttributes } } - }; + return new Dictionary<string, MetricConfig> { + { TelemetryMeter.TokenExchangeCount, new MetricConfig { Attributes = new HashSet<string>(defaultAttributes) } }, + { TelemetryMeter.RequestDuration, new MetricConfig { Attributes = new HashSet<string>(defaultAttributes) } }, + { TelemetryMeter.QueryDuration, new MetricConfig { Attributes = new HashSet<string>(defaultAttributes) } }, + { TelemetryMeter.HttpRequestDuration, new MetricConfig { Attributes = new HashSet<string>(defaultAttributes) } }, + // { TelemetryMeters.RequestCount, new MetricConfig { Attributes = new HashSet<string>(defaultAttributes) } } + };src/OpenFga.Sdk/Telemetry/Metrics.cs (1)
104-131: Reduce per-request attribute computation for HttpRequestDuration.
BuildForHttpRequestonly emits one metric, so computing attributes for all enabled metrics is extra work on a hot path. Consider building attributes only forhttpRequestDurationConfig.Attributes.♻️ Suggested update
- // Compute all enabled attributes once - var attributes = Attributes.BuildAttributesForResponse( - _allEnabledAttributes, _credentialsConfig, apiName, response, requestBuilder, httpRequestDuration, retryCount); + // Compute only the attributes needed for this metric + var enabledAttributes = httpRequestDurationConfig?.Attributes ?? new HashSet<string>(); + var attributes = Attributes.BuildAttributesForResponse( + enabledAttributes, _credentialsConfig, apiName, response, requestBuilder, httpRequestDuration, retryCount);
There was a problem hiding this comment.
Pull request overview
This PR adds a new telemetry metric fga-client.http_request.duration to track the duration of individual HTTP requests made by the SDK. This complements the existing fga-client.request.duration metric which tracks the total request time including retries and token exchanges.
Changes:
- Added new
HttpRequestDurationhistogram metric that records individual HTTP call durations - Modified
BaseClient.SendRequestAsyncto record HTTP request metrics before retrying - Updated telemetry configuration to enable the new metric by default
- Updated documentation and examples to include the new metric
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/OpenFga.Sdk/Telemetry/Meters.cs | Added HttpRequestDuration constant for the new metric name |
| src/OpenFga.Sdk/Telemetry/Histograms.cs | Added HttpRequestDurationHistogram and RecordHttpRequestDuration method |
| src/OpenFga.Sdk/Telemetry/Metrics.cs | Added BuildForHttpRequest method to record HTTP request metrics |
| src/OpenFga.Sdk/Configuration/TelemetryConfig.cs | Added default configuration for HttpRequestDuration metric |
| src/OpenFga.Sdk/ApiClient/BaseClient.cs | Modified to accept Metrics instance and record HTTP request duration; refactored SendRequestAsync to include metric recording |
| src/OpenFga.Sdk/ApiClient/ApiClient.cs | Updated to pass Metrics instance to BaseClient constructor |
| example/OpenTelemetryExample/OpenTelemetryExample.cs | Added example configuration for the new metric |
| OpenTelemetry.md | Updated documentation to include new metric and fixed typos in example code |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SoulPancake
force-pushed
the
feat/http-req-metric
branch
from
46fff01 to
f7289b3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/OpenFga.Sdk/ApiClient/OAuth2Client.cs:198
- The
Retrymethod in OAuth2Client does not set theretryCountfield on the returnedResponseWrapper<AccessTokenResponse>. This means that whenBuildForClientCredentialsResponseis called at line 154-155,accessTokenResponse.retryCountwill always be 0, even if retries occurred.
This is inconsistent with the ApiClient.Retry implementation which correctly sets response.retryCount = requestCount - 1 at lines 178-179 of ApiClient.cs. The OAuth2Client should follow the same pattern to ensure accurate retry count metrics for token exchange requests.
private async Task<TResult> Retry<TResult>(Func<int, Task<TResult>> retryable, CancellationToken cancellationToken = default) {
var attemptCount = 0; // 0 = initial request, 1+ = retry attempts
while (true) {
try {
return await retryable(attemptCount).ConfigureAwait(false);
}
catch (FgaApiError err) when (err is FgaApiRateLimitExceededError || err.ShouldRetry) {
// Check if we should retry based on status code and attempt count
if (!_retryHandler.ShouldRetry(err.StatusCode, attemptCount)) {
err.RetryAttempt = attemptCount;
if (err.ResponseHeaders != null) {
var info = _retryHandler.ExtractRetryAfterInfoFromHeaders(err.ResponseHeaders);
err.RetryAfter = info.retryAfterSeconds;
err.RetryAfterRaw = info.retryAfterRaw;
}
throw;
}
// Calculate delay using Retry-After header or exponential backoff
var delay = _retryHandler.CalculateDelayFromHeaders(err.ResponseHeaders, attemptCount);
await Task.Delay(delay, cancellationToken).ConfigureAwait(false);
attemptCount++;
}
catch (Exception ex) when (_retryHandler.IsTransientError(ex, attemptCount)) {
// Network error - retry with exponential backoff (no headers available)
var delay = _retryHandler.CalculateDelayFromHeaders(null, attemptCount);
await Task.Delay(delay, cancellationToken).ConfigureAwait(false);
attemptCount++;
}
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rhamzeh
left a comment
rhamzeh
left a comment
There was a problem hiding this comment.
@SoulPancake I think we've made a mistake in openfga/js-sdk#303
This is not the metric we wanted, and the .NET SDK already had that - the fga-client.request.count - it's that that we need copied over to the rest.
Now that we added duration, we can keep it, but can we make sure that it is disabled by default? In general let's not add more by default and give users the opportunity to enable it if they need to.
But now we need to add tickets for JS and the other SDKs to replace the tickets that were closed.
Side-note: for this PR, it is good so long as we mark it as disabled by default.
Also - can you make sure to update the CHANGELOG?
|
Thanks for the context @rhamzeh Looking back at issue #418 and the equivalent tickets across the other SDKs, the description pointed toward |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Description
solves #79
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.