Conversation
|
Related Documentation 1 document(s) may need updating based on files changed in this PR: OpenFGA's Space OpenTelemetry Metrics IntegrationView Suggested Changes@@ -36,7 +36,7 @@
| `url.full` | string | (JS: No, Java: Yes)| Full URL of the request | request.duration, query.duration, http_request.duration |
| `url.scheme` | string | (JS: No, Java: Yes)| HTTP scheme (`http`/`https`) | request.duration, query.duration, http_request.duration |
| `http.request.resend_count` | int | Yes | Number of retries attempted | request.duration, query.duration |
-| `fga-client.request.batch_check_size` | int | No (Java only) | Number of objects in a batch check request | request.duration, query.duration |
+| `fga-client.request.batch_check_size` | int | No | Number of checks in a batch check request | request.duration, query.duration |
| `http.client.request.duration` | int | No (JS only) | Time taken by the FGA server to process and evaluate the request (rounded to ms) | |
| `http.server.request.duration` | int | No (JS only) | Number of retries attempted | |
@@ -225,6 +225,7 @@
- If OpenTelemetry is not configured, metrics are not sent (no-op).
- The Java SDK supports both manual configuration and Java Agent (automatic instrumentation with zero code changes).
- High-cardinality attributes (like `fga-client.user`) are disabled by default to avoid excessive costs with some metric collectors. Enable only if necessary.
+- The `fga-client.request.batch_check_size` attribute is disabled by default in both Java and .NET SDKs as it only applies to BatchCheck requests. It can be explicitly enabled in the telemetry configuration if needed for monitoring batch check request sizes.
---
Note: You must be authenticated to accept/decline updates. How did I do? Any feedback? |
There was a problem hiding this comment.
Pull request overview
Adds a new optional OpenTelemetry attribute to report the number of checks in BatchCheck requests, with corresponding documentation and configuration tests.
Changes:
- Introduces
fga-client.request.batch_check_sizeas a supported telemetry attribute and includes it in the supported attribute set. - Extracts the
checksarray length fromBatchCheckrequest bodies (when the attribute is enabled). - Updates OpenTelemetry docs and adds tests around default telemetry configuration behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/OpenFga.Sdk/Telemetry/Attributes.cs |
Adds the new attribute constant, registers it as supported, and implements best-effort extraction for BatchCheck requests. |
src/OpenFga.Sdk/Configuration/TelemetryConfig.cs |
Notes that the new attribute is not enabled by default. |
src/OpenFga.Sdk.Test/Configuration/TelemetryConfigTests.cs |
Adds tests validating default telemetry metrics/attributes and explicit enabling. |
OpenTelemetry.md |
Documents the new attribute in the supported attributes table. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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:
WalkthroughThis PR introduces batch check size tracking as a new telemetry attribute by adding a RequestBatchCheckSize metric attribute that captures the number of items in BatchCheck API requests. The implementation includes JSON-based extraction of batch size from request bodies, configuration options to enable/disable the attribute, and comprehensive test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/OpenFga.Sdk.Test/Configuration/TelemetryConfigTests.cs (1)
87-87: Nit: redundant fully-qualified namespace.
System.Collections.Generic.HashSet<string>can be shortened toHashSet<string>sinceusing System.Collections.Generic;is already imported at line 14.Proposed fix
- Attributes = new System.Collections.Generic.HashSet<string> { + Attributes = new HashSet<string> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenFga.Sdk.Test/Configuration/TelemetryConfigTests.cs` at line 87, Replace the redundant fully-qualified type used to initialize Attributes in TelemetryConfigTests: change "System.Collections.Generic.HashSet<string>" to the shorter "HashSet<string>" (the Attributes property initialization inside the test method/class where Attributes = new System.Collections.Generic.HashSet<string> is set), since "using System.Collections.Generic;" is already present.src/OpenFga.Sdk/Telemetry/Attributes.cs (1)
256-276: Consider cachingrequestBuilder.JsonBodyto avoid redundant serialization.
RequestBuilder.JsonBodyis a computed property that callsJsonSerializer.Serialize(Body)on every access. Today, for a"BatchCheck"request, this method is the only caller, so there's a single serialization. However,AddRequestBodyAttributes(line 230) follows the same pattern, and if the gating conditions ever overlap for a request type, the body will be serialized twice.A minor improvement would be to serialize once in
AddRequestAttributesand pass the JSON string down to both helpers. This is low-priority since the current code path for"BatchCheck"only triggers one serialization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenFga.Sdk/Telemetry/Attributes.cs` around lines 256 - 276, The current AddBatchCheckSizeAttribute accesses RequestBuilder.JsonBody which re-serializes Body; to avoid duplicate serialization, change AddRequestAttributes to serialize requestBuilder.JsonBody once into a local string and pass that JSON string into both AddBatchCheckSizeAttribute and AddRequestBodyAttributes (update their signatures to accept a jsonBody string parameter, e.g., AddBatchCheckSizeAttribute<T>(RequestBuilder<T> requestBuilder, TagList attributes, string? jsonBody)); inside AddBatchCheckSizeAttribute use the passed jsonBody instead of reading requestBuilder.JsonBody and keep the same try/catch parsing logic.OpenTelemetry.md (1)
29-31: Minor: inconsistent table column widths.Lines 29–31 use wider formatting (longer attribute names with extra padding) compared to the surrounding rows (lines 25–28, 32–38), breaking the visual alignment of the Markdown source. Most renderers handle this fine, but aligning the column separator on line 23 to accommodate the new longest attribute name (
fga-client.request.batch_check_size) would keep the source tidy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@OpenTelemetry.md` around lines 29 - 31, Adjust the Markdown table column widths so the separators align with the longest attribute name by widening the header separator row to match the length of `fga-client.request.batch_check_size`; update the padding for the cells containing `fga-client.request.client_id`, `fga-client.request.batch_check_size`, and `fga-client.user` so the pipe characters line up with the surrounding rows and restore consistent column alignment in OpenTelemetry.md.src/OpenFga.Sdk.Test/Telemetry/AttributesTests.cs (1)
48-81: Consider adding InlineData(0) to cover the empty batch edge case.The parameterized test covers 1, 3, and 10 items. An empty
checksarray (count = 0) is a valid boundary condition — the code will add the attribute with value0. If that's the desired behavior, anInlineData(0)test documents it; if not, the code should skip adding the attribute for empty arrays.Proposed addition
[Theory] +[InlineData(0)] [InlineData(1)] [InlineData(3)] [InlineData(10)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OpenFga.Sdk.Test/Telemetry/AttributesTests.cs` around lines 48 - 81, The parameterized test BuildAttributesForResponse_BatchCheck_IncludesBatchCheckSizeAttribute is missing the empty-batch boundary case; add [InlineData(0)] to the Theory so the test runs with checkCount = 0, keep the rest of the test logic (BuildChecks, BatchCheckRequest, RequestBuilder, Attributes.BuildAttributesForResponse) and assert that the TelemetryAttribute.RequestBatchCheckSize tag exists and its value equals 0, or if the intended behavior is to omit the attribute for empty batches, update the test to assert absence of the tag instead (adjust expected assertion accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@OpenTelemetry.md`:
- Around line 29-31: Adjust the Markdown table column widths so the separators
align with the longest attribute name by widening the header separator row to
match the length of `fga-client.request.batch_check_size`; update the padding
for the cells containing `fga-client.request.client_id`,
`fga-client.request.batch_check_size`, and `fga-client.user` so the pipe
characters line up with the surrounding rows and restore consistent column
alignment in OpenTelemetry.md.
In `@src/OpenFga.Sdk.Test/Configuration/TelemetryConfigTests.cs`:
- Line 87: Replace the redundant fully-qualified type used to initialize
Attributes in TelemetryConfigTests: change
"System.Collections.Generic.HashSet<string>" to the shorter "HashSet<string>"
(the Attributes property initialization inside the test method/class where
Attributes = new System.Collections.Generic.HashSet<string> is set), since
"using System.Collections.Generic;" is already present.
In `@src/OpenFga.Sdk.Test/Telemetry/AttributesTests.cs`:
- Around line 48-81: The parameterized test
BuildAttributesForResponse_BatchCheck_IncludesBatchCheckSizeAttribute is missing
the empty-batch boundary case; add [InlineData(0)] to the Theory so the test
runs with checkCount = 0, keep the rest of the test logic (BuildChecks,
BatchCheckRequest, RequestBuilder, Attributes.BuildAttributesForResponse) and
assert that the TelemetryAttribute.RequestBatchCheckSize tag exists and its
value equals 0, or if the intended behavior is to omit the attribute for empty
batches, update the test to assert absence of the tag instead (adjust expected
assertion accordingly).
In `@src/OpenFga.Sdk/Telemetry/Attributes.cs`:
- Around line 256-276: The current AddBatchCheckSizeAttribute accesses
RequestBuilder.JsonBody which re-serializes Body; to avoid duplicate
serialization, change AddRequestAttributes to serialize requestBuilder.JsonBody
once into a local string and pass that JSON string into both
AddBatchCheckSizeAttribute and AddRequestBodyAttributes (update their signatures
to accept a jsonBody string parameter, e.g.,
AddBatchCheckSizeAttribute<T>(RequestBuilder<T> requestBuilder, TagList
attributes, string? jsonBody)); inside AddBatchCheckSizeAttribute use the passed
jsonBody instead of reading requestBuilder.JsonBody and keep the same try/catch
parsing logic.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
OpenTelemetry.mdsrc/OpenFga.Sdk.Test/Configuration/TelemetryConfigTests.cssrc/OpenFga.Sdk.Test/Telemetry/AttributesTests.cssrc/OpenFga.Sdk/Configuration/TelemetryConfig.cssrc/OpenFga.Sdk/Telemetry/Attributes.cs
Description
similar to the openfga/java-sdk#143
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit
New Features
Documentation
Tests