fix: share single Telemetry instance per SDK client#290
fix: share single Telemetry instance per SDK client#290jimmyjames wants to merge 2 commits intomainfrom
Conversation
|
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 addresses issue Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (37.80%) 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 #290 +/- ##
============================================
+ Coverage 37.70% 37.80% +0.09%
- Complexity 1236 1243 +7
============================================
Files 197 197
Lines 7609 7621 +12
Branches 880 883 +3
============================================
+ Hits 2869 2881 +12
Misses 4601 4601
Partials 139 139 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/dev/openfga/sdk/api/client/HttpRequestAttemptRetryTest.java (1)
484-485:⚠️ Potential issue | 🟡 MinorMissing telemetry parameter - inconsistent with other tests.
The
shouldRetryOnConnectionTimeouttest uses the old 5-parameterHttpRequestAttemptconstructor without passingTelemetry, while all other tests in this file were updated to use the new 6-parameter constructor with telemetry. This appears to be an oversight.🔧 Proposed fix
HttpRequestAttempt<Void> attempt = - new HttpRequestAttempt<>(request, "test", Void.class, apiClient, timeoutConfig); + new HttpRequestAttempt<>(request, "test", Void.class, apiClient, timeoutConfig, new Telemetry(timeoutConfig));
🤖 Fix all issues with AI agents
In `@src/main/java/dev/openfga/sdk/api/OpenFgaApi.java`:
- Around line 83-87: The OpenFgaApi constructor OpenFgaApi(Configuration,
ApiClient, Telemetry) lacks a null check for telemetry; add a validation at the
start of that constructor that throws IllegalArgumentException if telemetry is
null (matching ApiExecutor behavior) so subsequent uses of telemetry (e.g., in
HttpRequestAttempt calls) cannot cause a NullPointerException.
🧹 Nitpick comments (2)
src/test/java/dev/openfga/sdk/api/client/ApiExecutorTest.java (1)
385-391: Test verifies backward compatibility but could be more robust.The test confirms the 2-parameter constructor works without throwing, which validates backward compatibility. However, consider expanding it to verify the internally-created
Telemetryis functional:💡 Optional: Add verification that the executor is functional
`@Test` public void twoParamConstructor_shouldCreateWithOwnTelemetry() throws Exception { // Verifies the backward-compatible 2-param constructor works ClientConfiguration config = new ClientConfiguration().apiUrl(fgaApiUrl).storeId(DEFAULT_STORE_ID); ApiExecutor executor = new ApiExecutor(new ApiClient(), config); assertNotNull(executor); + // Optionally verify it can build a request (doesn't require actual HTTP call) + assertNotNull(executor.getClass().getDeclaredField("telemetry")); }src/main/java/dev/openfga/sdk/api/client/OpenFgaClient.java (1)
80-84: Consider thread-safety for concurrent configuration updates.When
setConfigurationis called, bothtelemetryandapifields are reassigned in two separate statements (lines 82-83). IfOpenFgaClientis used concurrently from multiple threads, a thread could observe an inconsistent state wheretelemetryis updated butapistill references the oldOpenFgaApi(or vice versa).If concurrent access to
OpenFgaClientis expected, consider either:
- Making the updates atomic using synchronization
- Documenting that
setConfigurationis not thread-safe and should not be called concurrently with API operations
Instead of creating new Telemetry, Metrics, and OpenTelemetry instruments for every HTTP request, share a single Telemetry instance per SDK client. This avoids wasted allocation, fragmented OTel instrument instances, and repeated GlobalOpenTelemetry meter lookups. Also fixes thread-safety bugs that become relevant once the instance is shared across concurrent async requests: - Telemetry.metrics(): volatile field + double-checked locking - Metrics counters/histograms: HashMap → ConcurrentHashMap with computeIfAbsent All existing public constructors are preserved for backward compatibility. Closes #209
jimmyjames
force-pushed
the
fix/209-share-telemetry-instance
branch
from
b5251de to
d2058e8
Compare
|
Related Documentation Checked 8 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
There was a problem hiding this comment.
Pull request overview
This PR fixes SDK telemetry/metrics being re-created per HTTP request by introducing a shared Telemetry instance per SDK client and making Telemetry/Metrics safe for concurrent use, addressing issue #209.
Changes:
- Share a single
Telemetryinstance across request execution paths (OpenFgaClient→OpenFgaApi/ApiExecutor→HttpRequestAttempt) and withinOAuth2Client. - Make lazy
Telemetry.metrics()initialization thread-safe (double-checked locking) and makeMetricsinstrument caches concurrent (ConcurrentHashMap+computeIfAbsent). - Add/adjust tests for singleton behavior, concurrent access, constructor compatibility, and null telemetry validation.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/dev/openfga/sdk/telemetry/Telemetry.java | Thread-safe lazy initialization of a shared Metrics instance. |
| src/main/java/dev/openfga/sdk/telemetry/Metrics.java | Make counter/histogram caches concurrency-safe and atomically initialized. |
| src/main/java/dev/openfga/sdk/api/client/OpenFgaClient.java | Own and propagate a single Telemetry instance per client. |
| src/main/java/dev/openfga/sdk/api/OpenFgaApi.java | Accept and reuse injected Telemetry for all request attempts; add null validation. |
| src/main/java/dev/openfga/sdk/api/client/ApiExecutor.java | Accept and reuse injected Telemetry; preserve 2-param constructor via delegation. |
| src/main/java/dev/openfga/sdk/api/client/HttpRequestAttempt.java | Add telemetry-injecting constructor and enforce non-null telemetry. |
| src/main/java/dev/openfga/sdk/api/auth/OAuth2Client.java | Reuse the client’s own telemetry when issuing token exchange requests. |
| src/test/java/dev/openfga/sdk/telemetry/TelemetryTest.java | Add concurrent access test for Telemetry.metrics() singleton behavior. |
| src/test/java/dev/openfga/sdk/api/client/HttpRequestAttemptRetryTest.java | Update call sites to pass telemetry; add null telemetry rejection test. |
| src/test/java/dev/openfga/sdk/api/client/ApiExecutorTest.java | Add tests for backward-compatible constructor and null telemetry validation. |
| src/test/java/dev/openfga/sdk/api/OpenFgaApiTest.java | Add null telemetry rejection test for new constructor overload. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // when | ||
| for (int i = 0; i < threadCount; i++) { | ||
| executor.submit(() -> { | ||
| try { | ||
| startLatch.await(); | ||
| results.add(telemetry.metrics()); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
| }); | ||
| } | ||
| startLatch.countDown(); | ||
| executor.shutdown(); | ||
| executor.awaitTermination(5, java.util.concurrent.TimeUnit.SECONDS); | ||
|
|
||
| // then | ||
| assertThat(results).hasSize(threadCount); | ||
| Metrics expected = results.get(0); | ||
| assertThat(results).allSatisfy(m -> assertThat(m).isSameAs(expected)); |
There was a problem hiding this comment.
The test doesn't verify that awaitTermination(...) actually completed and doesn't ensure the ExecutorService is cleaned up if the test fails early. This can lead to flaky assertions (results not fully populated) and potential thread leakage in CI. Consider asserting the awaitTermination result and using a try/finally to call shutdownNow() when needed.
| // when | |
| for (int i = 0; i < threadCount; i++) { | |
| executor.submit(() -> { | |
| try { | |
| startLatch.await(); | |
| results.add(telemetry.metrics()); | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| } | |
| }); | |
| } | |
| startLatch.countDown(); | |
| executor.shutdown(); | |
| executor.awaitTermination(5, java.util.concurrent.TimeUnit.SECONDS); | |
| // then | |
| assertThat(results).hasSize(threadCount); | |
| Metrics expected = results.get(0); | |
| assertThat(results).allSatisfy(m -> assertThat(m).isSameAs(expected)); | |
| try { | |
| // when | |
| for (int i = 0; i < threadCount; i++) { | |
| executor.submit(() -> { | |
| try { | |
| startLatch.await(); | |
| results.add(telemetry.metrics()); | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| } | |
| }); | |
| } | |
| startLatch.countDown(); | |
| executor.shutdown(); | |
| boolean terminated = executor.awaitTermination(5, java.util.concurrent.TimeUnit.SECONDS); | |
| assertThat(terminated).isTrue(); | |
| // then | |
| assertThat(results).hasSize(threadCount); | |
| Metrics expected = results.get(0); | |
| assertThat(results).allSatisfy(m -> assertThat(m).isSameAs(expected)); | |
| } finally { | |
| if (!executor.isTerminated()) { | |
| executor.shutdownNow(); | |
| } | |
| } |
There was a problem hiding this comment.
Addressed in 34cfbe2 — added try/finally with shutdownNow() cleanup and an assertion on the awaitTermination return value. Thanks for the catch!
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/java/dev/openfga/sdk/telemetry/TelemetryTest.java (1)
49-49: Nit: importTimeUnitinstead of using FQN.Lines 6–10 already import other
java.util.concurrenttypes. For consistency, importTimeUnitas well.import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit;Then on line 49:
- executor.awaitTermination(5, java.util.concurrent.TimeUnit.SECONDS); + executor.awaitTermination(5, TimeUnit.SECONDS);
Assert awaitTermination returns true for clear timeout failures, and wrap test body in try/finally with shutdownNow to prevent thread leaks.
jimmyjames
force-pushed
the
fix/209-share-telemetry-instance
branch
from
289a4ae to
34cfbe2
Compare
2 participants
Description
Fixes #209
The SDK was creating new
Telemetry,Metrics, and associated OpenTelemetry instruments for every HTTP request instead of sharing a single instance per SDK client. This caused:GlobalOpenTelemetrymeter lookupsAdditionally, once a single
Telemetry/Metricsinstance is shared across concurrent async requests, the existing code had thread-safety bugs that are fixed here.Changes
Thread safety (
Telemetry.java,Metrics.java):Telemetry.metrics():volatilefield + double-checked locking for safe lazy initMetrics:HashMap→ConcurrentHashMap, check-then-put →computeIfAbsent()for counters and histogramsShared instance wiring (
OpenFgaClient.java,OpenFgaApi.java,ApiExecutor.java,OAuth2Client.java,HttpRequestAttempt.java):OpenFgaClientowns the singleTelemetryinstance, passes it toOpenFgaApiandApiExecutorOpenFgaApipasses shared telemetry through to allHttpRequestAttemptcallsOAuth2Clientpasses its ownTelemetry(one per client, not per request) toHttpRequestAttempttelemetryparameter for null, consistent with existing parameter validationBackward compatibility:
HttpRequestAttempt(5-param)delegates to newHttpRequestAttempt(6-param)ApiExecutor(2-param)delegates to newApiExecutor(3-param)OpenFgaApi(1-param)andOpenFgaApi(2-param)preserved, newOpenFgaApi(3-param)addedTest plan
TelemetryTest.shouldBeASingletonMetricsInitializationvalidates singleton behaviorTelemetry.metrics()returns same instance across 10 threadsApiExecutor2-param constructorHttpRequestAttemptRetryTestupdated to use shared telemetry instances./gradlew checkpasses (tests + spotless formatting)Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests