feat: implement telemetry singleton pattern to reuse telemetry from sdk client root #213
Conversation
…ross SDK components
|
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 WalkthroughTelemetry is now retrieved from Configuration and injected into OAuth2Client and HttpRequestAttempt. OpenFgaApi fetches telemetry via configuration.getTelemetry() and passes it through all HTTP request paths. Configuration lazily provides a singleton Telemetry per instance. Tests added/updated to validate telemetry sharing and isolation across configurations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Cfg as Configuration
participant Api as OpenFgaApi
participant OAuth as OAuth2Client
participant Req as HttpRequestAttempt
participant Http as ApiClient
participant Tel as Telemetry
App->>Cfg: create Configuration(...)
App->>Api: new OpenFgaApi(Cfg)
Api->>Cfg: getTelemetry()
Cfg-->>Api: Tel (singleton per Configuration)
note over Api,OAuth: Telemetry is injected downstream
Api->>OAuth: new OAuth2Client(Cfg, Http, Tel)
App->>Api: perform API call
Api->>Req: new HttpRequestAttempt(request, name, clazz, Http, Cfg, Tel)
Req->>Http: execute()
Http-->>Req: response
Req-->>Api: parsed result
Api-->>App: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (11)
src/test/java/dev/openfga/sdk/api/OpenFgaApiTest.java (1)
78-79: Prefer explicit singleton stub for clarityFunctionally correct, but making the singleton intent explicit improves readability.
- when(mockConfiguration.getTelemetry()).thenReturn(new Telemetry(mockConfiguration)); + Telemetry telemetry = new Telemetry(mockConfiguration); + when(mockConfiguration.getTelemetry()).thenReturn(telemetry);src/main/java/dev/openfga/sdk/api/configuration/Configuration.java (2)
58-59: Publish Telemetry safely across threads (mark field volatile)Without volatile/synchronization, another thread may not see the initialized reference promptly. Marking it volatile supports the double-checked locking pattern or at least safe publication.
- private Telemetry telemetry; + private volatile Telemetry telemetry;
354-359: Make lazy singleton initialization thread-safeThe current lazy init is not thread-safe. Two threads could race to create different Telemetry instances, and one overwrites the other. Use double-checked locking with volatile field to ensure a single instance is created and safely published.
- public Telemetry getTelemetry() { - if (telemetry == null) { - telemetry = new Telemetry(this); - } - return telemetry; - } + public Telemetry getTelemetry() { + Telemetry instance = telemetry; + if (instance == null) { + synchronized (this) { + instance = telemetry; + if (instance == null) { + instance = new Telemetry(this); + telemetry = instance; + } + } + } + return instance; + }src/main/java/dev/openfga/sdk/api/client/HttpRequestAttempt.java (2)
36-47: Reduce duplication by constructor chaining and validate Telemetry param
- Delegate the primary ctor to the new overload to avoid duplication.
- Validate the provided Telemetry is non-null to prevent NPEs in metrics() calls if external callers use the new public overload.
- public HttpRequestAttempt( - HttpRequest request, String name, Class<T> clazz, ApiClient apiClient, Configuration configuration) - throws FgaInvalidParameterException { - assertParamExists(configuration.getMaxRetries(), "maxRetries", "Configuration"); - this.apiClient = apiClient; - this.configuration = configuration; - this.name = name; - this.request = request; - this.clazz = clazz; - this.telemetry = configuration.getTelemetry(); - this.telemetryAttributes = new HashMap<>(); - } + public HttpRequestAttempt( + HttpRequest request, String name, Class<T> clazz, ApiClient apiClient, Configuration configuration) + throws FgaInvalidParameterException { + this(request, name, clazz, apiClient, configuration, configuration.getTelemetry()); + } public HttpRequestAttempt( HttpRequest request, String name, Class<T> clazz, ApiClient apiClient, Configuration configuration, Telemetry telemetry) throws FgaInvalidParameterException { assertParamExists(configuration.getMaxRetries(), "maxRetries", "Configuration"); + assertParamExists(telemetry, "telemetry", "HttpRequestAttempt"); this.apiClient = apiClient; this.configuration = configuration; this.name = name; this.request = request; this.clazz = clazz; this.telemetry = telemetry; this.telemetryAttributes = new HashMap<>(); }Also applies to: 49-60
66-69: Guard against null in setTelemetryAttributesDefensive check prevents null map leading to NPE when adding attributes later.
- public HttpRequestAttempt<T> setTelemetryAttributes(Map<Attribute, String> attributes) { - this.telemetryAttributes = attributes; + public HttpRequestAttempt<T> setTelemetryAttributes(Map<Attribute, String> attributes) { + this.telemetryAttributes = (attributes != null) ? attributes : new HashMap<>(); return this; }src/test/java/dev/openfga/sdk/api/configuration/ConfigurationTelemetryTest.java (1)
20-67: Optionally add a concurrency test for lazy initA simple multi-threaded test would detect any race in lazy initialization.
// Add to this test class @Test void shouldBeThreadSafeOnGetTelemetry() throws Exception { Configuration configuration = new Configuration(); var pool = java.util.concurrent.Executors.newFixedThreadPool(8); try { var futures = new java.util.ArrayList<java.util.concurrent.Future<Telemetry>>(); for (int i = 0; i < 32; i++) { futures.add(pool.submit(configuration::getTelemetry)); } Telemetry first = null; for (var f : futures) { Telemetry t = f.get(); if (first == null) first = t; assertThat(t).isSameAs(first); } } finally { pool.shutdownNow(); } }src/main/java/dev/openfga/sdk/api/auth/OAuth2Client.java (1)
53-68: Validate telemetry parameter to avoid NPEs downstreamSince this overload is public, callers could pass null and trigger NPE when recording metrics. Add a null-validation.
- public OAuth2Client(Configuration configuration, ApiClient apiClient, Telemetry telemetry) throws FgaInvalidParameterException { + public OAuth2Client(Configuration configuration, ApiClient apiClient, Telemetry telemetry) throws FgaInvalidParameterException { var clientCredentials = configuration.getCredentials().getClientCredentials(); this.apiClient = apiClient; this.authRequest = new CredentialsFlowRequest(clientCredentials.getClientId(), clientCredentials.getClientSecret()); this.authRequest.setAudience(clientCredentials.getApiAudience()); this.authRequest.setScope(clientCredentials.getScopes()); this.config = new Configuration() .apiUrl(buildApiTokenIssuer(clientCredentials.getApiTokenIssuer())) .connectTimeout(configuration.getConnectTimeout()) .maxRetries(configuration.getMaxRetries()) .minimumRetryDelay(configuration.getMinimumRetryDelay()) .telemetryConfiguration(configuration.getTelemetryConfiguration()); - this.telemetry = telemetry; + if (telemetry == null) { + throw new FgaInvalidParameterException("telemetry", "OAuth2Client"); + } + this.telemetry = telemetry; }Add this import at the top if you prefer standard null checks instead:
import java.util.Objects;Then replace the if-block with:
this.telemetry = Objects.requireNonNull(telemetry, "telemetry");src/test/java/dev/openfga/sdk/api/integration/TelemetrySingletonIntegrationTest.java (4)
35-48: Strengthen the assertion: verify identity, not just non-nullThis test currently only checks non-null. Assert that Configuration returns a singleton and that OpenFgaApi is wired to the same instance via reflection.
Apply:
@@ void shouldUseSameTelemetryInstanceAcrossComponents() throws FgaInvalidParameterException { @@ - OpenFgaApi api = new OpenFgaApi(configuration); - Telemetry configTelemetry = configuration.getTelemetry(); + OpenFgaApi api = new OpenFgaApi(configuration); + Telemetry configTelemetry = configuration.getTelemetry(); + // calling again should return the same instance + Telemetry configTelemetry2 = configuration.getTelemetry(); @@ - assertThat(configTelemetry).isNotNull(); - // We can't directly access the telemetry field in OpenFgaApi, but we've verified in code - // that it uses configuration.getTelemetry() + assertThat(configTelemetry).isNotNull(); + assertThat(configTelemetry2).isSameAs(configTelemetry); + // Verify OpenFgaApi is using the same telemetry instance + try { + var field = OpenFgaApi.class.getDeclaredField("telemetry"); + field.setAccessible(true); + Telemetry apiTelemetry = (Telemetry) field.get(api); + assertThat(apiTelemetry).isSameAs(configTelemetry); + } catch (ReflectiveOperationException e) { + throw new AssertionError("Failed to reflect telemetry field from OpenFgaApi", e); + } }And add the missing import:
@@ -import java.time.Duration; +import java.lang.reflect.Field;
51-76: Verify OAuth2Client uses the shared configuration telemetrySimilarly, assert that the 2-arg OAuth2Client constructor leverages configuration.getTelemetry() by reflecting the private field.
@@ - OpenFgaApi api = new OpenFgaApi(configuration, apiClient); - OAuth2Client oauth2Client = new OAuth2Client(configuration, apiClient); - Telemetry configTelemetry = configuration.getTelemetry(); + OpenFgaApi api = new OpenFgaApi(configuration, apiClient); + OAuth2Client oauth2Client = new OAuth2Client(configuration, apiClient); + Telemetry configTelemetry = configuration.getTelemetry(); @@ - assertThat(configTelemetry).isNotNull(); - // The OAuth2Client and OpenFgaApi both use the same telemetry instance from configuration + assertThat(configTelemetry).isNotNull(); + // Verify OAuth2Client uses the same telemetry instance + try { + var field = OAuth2Client.class.getDeclaredField("telemetry"); + field.setAccessible(true); + Telemetry oauthTelemetry = (Telemetry) field.get(oauth2Client); + assertThat(oauthTelemetry).isSameAs(configTelemetry); + } catch (ReflectiveOperationException e) { + throw new AssertionError("Failed to reflect telemetry field from OAuth2Client", e); + }
78-96: Verify HttpRequestAttempt wiring uses the passed telemetry instanceThis test should also assert identity via reflection to guarantee the constructor didn’t allocate another telemetry.
@@ - Telemetry configTelemetry = configuration.getTelemetry(); - HttpRequestAttempt<Void> attempt = new HttpRequestAttempt<>( - request, "test", Void.class, apiClient, configuration, configTelemetry); + Telemetry configTelemetry = configuration.getTelemetry(); + HttpRequestAttempt<Void> attempt = new HttpRequestAttempt<>( + request, "test", Void.class, apiClient, configuration, configTelemetry); @@ - assertThat(configTelemetry).isNotNull(); - // The HttpRequestAttempt now receives the telemetry instance instead of creating a new one + assertThat(configTelemetry).isNotNull(); + // Ensure the attempt holds the same telemetry instance + try { + var field = HttpRequestAttempt.class.getDeclaredField("telemetry"); + field.setAccessible(true); + Telemetry attemptTelemetry = (Telemetry) field.get(attempt); + assertThat(attemptTelemetry).isSameAs(configTelemetry); + } catch (ReflectiveOperationException e) { + throw new AssertionError("Failed to reflect telemetry field from HttpRequestAttempt", e); + }
29-29: Remove unused importDuration is not used in this file.
-import java.time.Duration;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
src/main/java/dev/openfga/sdk/api/OpenFgaApi.java(18 hunks)src/main/java/dev/openfga/sdk/api/auth/OAuth2Client.java(3 hunks)src/main/java/dev/openfga/sdk/api/client/HttpRequestAttempt.java(1 hunks)src/main/java/dev/openfga/sdk/api/configuration/Configuration.java(4 hunks)src/test/java/dev/openfga/sdk/api/OpenFgaApiTest.java(2 hunks)src/test/java/dev/openfga/sdk/api/configuration/ConfigurationTelemetryTest.java(1 hunks)src/test/java/dev/openfga/sdk/api/integration/TelemetrySingletonIntegrationTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/test/java/dev/openfga/sdk/api/integration/TelemetrySingletonIntegrationTest.java (5)
src/main/java/dev/openfga/sdk/api/OpenFgaApi.java (1)
OpenFgaApi(67-1278)src/main/java/dev/openfga/sdk/api/auth/OAuth2Client.java (1)
OAuth2Client(28-130)src/main/java/dev/openfga/sdk/api/client/HttpRequestAttempt.java (1)
HttpRequestAttempt(23-268)src/main/java/dev/openfga/sdk/api/configuration/Configuration.java (1)
Configuration(33-360)src/main/java/dev/openfga/sdk/telemetry/Telemetry.java (1)
Telemetry(20-39)
src/test/java/dev/openfga/sdk/api/configuration/ConfigurationTelemetryTest.java (1)
src/main/java/dev/openfga/sdk/telemetry/Telemetry.java (1)
Telemetry(20-39)
src/test/java/dev/openfga/sdk/api/OpenFgaApiTest.java (1)
src/main/java/dev/openfga/sdk/telemetry/Telemetry.java (1)
Telemetry(20-39)
src/main/java/dev/openfga/sdk/api/configuration/Configuration.java (1)
src/main/java/dev/openfga/sdk/telemetry/Telemetry.java (1)
Telemetry(20-39)
🔇 Additional comments (7)
src/test/java/dev/openfga/sdk/api/OpenFgaApiTest.java (1)
25-25: LGTM: Telemetry import aligns with new wiringImporting Telemetry here is consistent with the PR objective to inject shared telemetry via Configuration.
src/main/java/dev/openfga/sdk/api/configuration/Configuration.java (1)
144-146: Good: avoid carrying over Telemetry on overrideNot copying the Telemetry instance ensures the new Configuration lazily builds its own, matching the design goal.
src/main/java/dev/openfga/sdk/api/client/HttpRequestAttempt.java (1)
45-47: LGTM: Pulling Telemetry from ConfigurationSourcing telemetry via configuration.getTelemetry() matches the singleton wiring across the SDK.
src/test/java/dev/openfga/sdk/api/configuration/ConfigurationTelemetryTest.java (1)
20-67: Solid coverage of Telemetry lifecycle semanticsTests validate same-instance per Configuration, different instances across Configurations, and override behavior. This aligns with the PR goals.
src/main/java/dev/openfga/sdk/api/auth/OAuth2Client.java (2)
42-45: LGTM: Constructor delegates to telemetry-aware overloadKeeps backward compatibility and centralizes initialization.
104-105: LGTM: Pass shared Telemetry to HttpRequestAttemptEnsures consistent telemetry across token exchange HTTP calls.
src/test/java/dev/openfga/sdk/api/integration/TelemetrySingletonIntegrationTest.java (1)
99-115: LGTM: verifies different Configuration instances yield distinct Telemetry singletonsThis test captures the intended isolation between configurations.
| public OpenFgaApi(Configuration configuration, ApiClient apiClient) throws FgaInvalidParameterException { | ||
| this.apiClient = apiClient; | ||
| this.configuration = configuration; | ||
| this.telemetry = new Telemetry(this.configuration); | ||
| this.telemetry = configuration.getTelemetry(); | ||
|
|
||
| if (configuration.getCredentials().getCredentialsMethod() == CredentialsMethod.CLIENT_CREDENTIALS) { | ||
| this.oAuth2Client = new OAuth2Client(configuration, apiClient); | ||
| this.oAuth2Client = new OAuth2Client(configuration, apiClient, this.telemetry); | ||
| } else { |
There was a problem hiding this comment.
Telemetry initialization: good reuse, but beware configurationOverride semantics drift
Using a single Telemetry instance from the root Configuration and injecting it into OAuth2Client is consistent with the PR goal of reuse. However, downstream calls that accept a ConfigurationOverride will still emit telemetry through the root instance, ignoring any telemetryConfiguration set on the override. This can lead to telemetry being recorded with the wrong configuration (e.g., disabled/enabled, different exporters, different attributes) when an override is used.
If overrides are intended to have their own telemetry semantics (Configuration.override() explicitly resets telemetry to lazily reinitialize), pass telemetry from the effective configuration per call rather than from the root.
Two viable options:
- Preferred: derive telemetry per method using configuration.getTelemetry() when constructing HttpRequestAttempt.
- Or: drop the explicit telemetry parameter and use the HttpRequestAttempt constructor that pulls from its Configuration.
See suggested call-site fixes in the next comment.
🤖 Prompt for AI Agents
In src/main/java/dev/openfga/sdk/api/OpenFgaApi.java around lines 78-85, you are
capturing telemetry from the root Configuration and passing it into
OAuth2Client, which causes ConfigurationOverride telemetry settings to be
ignored; change the code to stop using the stored root telemetry for downstream
calls — either (preferred) stop injecting telemetry into OAuth2Client and
instead derive telemetry per call via configuration.getTelemetry() when
constructing HttpRequestAttempt (or wherever requests are built), or modify
OAuth2Client to pull telemetry from the effective Configuration at the time of
the request (e.g., pass the Configuration or ConfigurationOverride into
OAuth2Client methods and call getTelemetry() there) so overrides’
telemetryConfiguration is honored.
|
|
||
| if (configuration.getCredentials().getCredentialsMethod() == CredentialsMethod.CLIENT_CREDENTIALS) { | ||
| this.oAuth2Client = new OAuth2Client(configuration, apiClient); | ||
| this.oAuth2Client = new OAuth2Client(configuration, apiClient, this.telemetry); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid NPE when a ConfigurationOverride switches to CLIENT_CREDENTIALS
getAccessToken(Configuration) uses the instance field oAuth2Client (built only from the root Configuration). If a per-call ConfigurationOverride specifies CredentialsMethod.CLIENT_CREDENTIALS while the root did not, oAuth2Client is null and a NullPointerException will occur.
Create/choose an OAuth2Client based on the effective configuration (root vs override). Example:
// replace existing getAccessToken with:
private String getAccessToken(Configuration configuration) throws ApiException {
CredentialsMethod credentialsMethod = configuration.getCredentials().getCredentialsMethod();
if (credentialsMethod == CredentialsMethod.API_TOKEN) {
return configuration.getCredentials().getApiToken().getToken();
}
if (credentialsMethod == CredentialsMethod.CLIENT_CREDENTIALS) {
try {
OAuth2Client client =
(configuration == this.configuration && this.oAuth2Client != null)
? this.oAuth2Client
: new OAuth2Client(configuration, apiClient, configuration.getTelemetry());
return client.getAccessToken().get();
} catch (Exception e) {
throw new ApiException(e);
}
}
throw new IllegalStateException("Configuration is invalid.");
}This preserves reuse for the root client while supporting overrides with different credentials.
Would you like me to push a follow-up PR adjusting getAccessToken and updating the call sites accordingly?
🏁 Script executed:
#!/bin/bash
set -eu
# Locate the OpenFgaApi.java file
file=$(fd OpenFgaApi.java -t f | head -n1)
echo "Inspecting: $file"
# Show where oAuth2Client is declared and used
echo -e "\n=== oAuth2Client references ==="
rg -n 'oAuth2Client' -C3 "$file"
# Show the getAccessToken method signature and body
echo -e "\n=== getAccessToken implementation ==="
rg -n 'private.*getAccessToken' -C0 "$file"
rg -n 'getAccessToken' -C10 "$file"Length of output: 3892
Ensure OAuth2Client is initialized for overrides using CLIENT_CREDENTIALS
getAccessToken(Configuration) currently always uses the field oAuth2Client, which is only non-null when the root configuration uses CLIENT_CREDENTIALS. If a per-call Configuration override switches to CLIENT_CREDENTIALS, oAuth2Client remains null, causing an NPE.
Please update the method to reuse the root client when appropriate and instantiate a new one for overrides:
• File: src/main/java/dev/openfga/sdk/api/OpenFgaApi.java
Location: private String getAccessToken(Configuration configuration) (around lines 1261–1268)
Suggested replacement:
-private String getAccessToken(Configuration configuration) throws ApiException {
- CredentialsMethod credentialsMethod = configuration.getCredentials().getCredentialsMethod();
-
- if (credentialsMethod == CredentialsMethod.API_TOKEN) {
- return configuration.getCredentials().getApiToken().getToken();
- }
-
- if (credentialsMethod == CredentialsMethod.CLIENT_CREDENTIALS) {
- try {
- return oAuth2Client.getAccessToken().get();
- } catch (Exception e) {
- throw new ApiException(e);
- }
- }
-
- throw new IllegalStateException("Configuration is invalid.");
-}
+private String getAccessToken(Configuration configuration) throws ApiException {
+ CredentialsMethod credentialsMethod = configuration.getCredentials().getCredentialsMethod();
+
+ if (credentialsMethod == CredentialsMethod.API_TOKEN) {
+ return configuration.getCredentials().getApiToken().getToken();
+ }
+
+ if (credentialsMethod == CredentialsMethod.CLIENT_CREDENTIALS) {
+ try {
+ OAuth2Client client =
+ (configuration == this.configuration && this.oAuth2Client != null)
+ ? this.oAuth2Client
+ : new OAuth2Client(configuration, apiClient, configuration.getTelemetry());
+ return client.getAccessToken().get();
+ } catch (Exception e) {
+ throw new ApiException(e);
+ }
+ }
+
+ throw new IllegalStateException("Configuration is invalid.");
+}This preserves reuse of the root client while safely supporting per-call overrides with different credential methods.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/dev/openfga/sdk/api/OpenFgaApi.java around the
getAccessToken(Configuration) method (approx lines 1261–1268), the code always
references the field oAuth2Client which can be null for per-call Configuration
overrides using CLIENT_CREDENTIALS; update the method to: check if the effective
configuration's auth method is CLIENT_CREDENTIALS, and if so reuse
this.oAuth2Client when the passed configuration is the same as the root
configuration (instantiating and assigning it if the root intends
CLIENT_CREDENTIALS but the field is null), otherwise create a new local
OAuth2Client with the override configuration and use it to get the token
(without assigning it to the field); for non-CLIENT_CREDENTIALS flows continue
using existing logic—this ensures root reuse and safe per-call overrides without
NPEs.
There was a problem hiding this comment.
@SoulPancake, great! The fix I suggested will prevent the NPE and ensure OAuth2Client works correctly with both root configurations and per-call overrides. Let me know if you'd like me to help with anything else related to this telemetry implementation.
| HttpRequest request = buildHttpRequest("POST", path, body, configuration); | ||
| return new HttpRequestAttempt<>(request, "batchCheck", BatchCheckResponse.class, apiClient, configuration) | ||
| return new HttpRequestAttempt<>(request, "batchCheck", BatchCheckResponse.class, apiClient, configuration, this.telemetry) | ||
| .addTelemetryAttributes(telemetryAttributes) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure per-call ConfigurationOverride’s telemetry is honored
All these call sites pass the root telemetry (this.telemetry), which prevents telemetryConfiguration provided via ConfigurationOverride from taking effect. Use the effective configuration’s telemetry instead.
Apply this pattern change across the listed lines:
- return new HttpRequestAttempt<>(request, "batchCheck", BatchCheckResponse.class, apiClient, configuration, this.telemetry)
+ return new HttpRequestAttempt<>(request, "batchCheck", BatchCheckResponse.class, apiClient, configuration, configuration.getTelemetry())- return new HttpRequestAttempt<>(request, "check", CheckResponse.class, apiClient, configuration, this.telemetry)
+ return new HttpRequestAttempt<>(request, "check", CheckResponse.class, apiClient, configuration, configuration.getTelemetry())- return new HttpRequestAttempt<>(request, "createStore", CreateStoreResponse.class, apiClient, configuration, this.telemetry)
+ return new HttpRequestAttempt<>(request, "createStore", CreateStoreResponse.class, apiClient, configuration, configuration.getTelemetry())- return new HttpRequestAttempt<>(request, "deleteStore", Void.class, apiClient, configuration, this.telemetry)
+ return new HttpRequestAttempt<>(request, "deleteStore", Void.class, apiClient, configuration, configuration.getTelemetry())- return new HttpRequestAttempt<>(request, "expand", ExpandResponse.class, apiClient, configuration, this.telemetry)
+ return new HttpRequestAttempt<>(request, "expand", ExpandResponse.class, apiClient, configuration, configuration.getTelemetry())- return new HttpRequestAttempt<>(request, "getStore", GetStoreResponse.class, apiClient, configuration, this.telemetry)
+ return new HttpRequestAttempt<>(request, "getStore", GetStoreResponse.class, apiClient, configuration, configuration.getTelemetry())- return new HttpRequestAttempt<>(request, "listObjects", ListObjectsResponse.class, apiClient, configuration, this.telemetry)
+ return new HttpRequestAttempt<>(request, "listObjects", ListObjectsResponse.class, apiClient, configuration, configuration.getTelemetry())- return new HttpRequestAttempt<>(request, "listStores", ListStoresResponse.class, apiClient, configuration, this.telemetry)
+ return new HttpRequestAttempt<>(request, "listStores", ListStoresResponse.class, apiClient, configuration, configuration.getTelemetry())- return new HttpRequestAttempt<>(request, "listUsers", ListUsersResponse.class, apiClient, configuration, this.telemetry)
+ return new HttpRequestAttempt<>(request, "listUsers", ListUsersResponse.class, apiClient, configuration, configuration.getTelemetry())- return new HttpRequestAttempt<>(request, "read", ReadResponse.class, apiClient, configuration, this.telemetry)
+ return new HttpRequestAttempt<>(request, "read", ReadResponse.class, apiClient, configuration, configuration.getTelemetry())- return new HttpRequestAttempt<>(
- request, "readAssertions", ReadAssertionsResponse.class, apiClient, configuration, this.telemetry)
+ return new HttpRequestAttempt<>(
+ request, "readAssertions", ReadAssertionsResponse.class, apiClient, configuration, configuration.getTelemetry())- return new HttpRequestAttempt<>(
- request,
- "readAuthorizationModel",
- ReadAuthorizationModelResponse.class,
- apiClient,
- configuration,
- this.telemetry)
+ return new HttpRequestAttempt<>(
+ request,
+ "readAuthorizationModel",
+ ReadAuthorizationModelResponse.class,
+ apiClient,
+ configuration,
+ configuration.getTelemetry())- return new HttpRequestAttempt<>(
- request,
- "readAuthorizationModels",
- ReadAuthorizationModelsResponse.class,
- apiClient,
- configuration,
- this.telemetry)
+ return new HttpRequestAttempt<>(
+ request,
+ "readAuthorizationModels",
+ ReadAuthorizationModelsResponse.class,
+ apiClient,
+ configuration,
+ configuration.getTelemetry())- return new HttpRequestAttempt<>(request, "readChanges", ReadChangesResponse.class, apiClient, configuration, this.telemetry)
+ return new HttpRequestAttempt<>(request, "readChanges", ReadChangesResponse.class, apiClient, configuration, configuration.getTelemetry())- return new HttpRequestAttempt<>(request, "write", Object.class, apiClient, configuration, this.telemetry)
+ return new HttpRequestAttempt<>(request, "write", Object.class, apiClient, configuration, configuration.getTelemetry())- return new HttpRequestAttempt<>(request, "writeAssertions", Void.class, apiClient, configuration, this.telemetry)
+ return new HttpRequestAttempt<>(request, "writeAssertions", Void.class, apiClient, configuration, configuration.getTelemetry())- return new HttpRequestAttempt<>(
- request,
- "writeAuthorizationModel",
- WriteAuthorizationModelResponse.class,
- apiClient,
- configuration,
- this.telemetry)
+ return new HttpRequestAttempt<>(
+ request,
+ "writeAuthorizationModel",
+ WriteAuthorizationModelResponse.class,
+ apiClient,
+ configuration,
+ configuration.getTelemetry())Also applies to: 205-205, 255-255, 304-304, 360-360, 410-410, 466-466, 517-517, 573-573, 629-629, 687-687, 749-751, 815-816, 901-902, 956-956, 1023-1024, 1085-1087
There was a problem hiding this comment.
Thanks for the PR @SoulPancake!
As coderabbit points out, this change would break the current behavior of supporting configuration override.
Ideally I think we should just have one telemetry per API client, and not support override of telemetry config per request, like the .net SDK, but that would be a breaking change requiring more thought.
If possible, for now I think we should ensure we maintain support for configuration override for telemetry, and if we define a path to remove that we can (later) proceed with deprecations.
Maybe we can create a telemetry instance per client, pass that through as needed, and use it unless the telemetry config is changed (if we can detect that)?
Also, fwiw, if we do have a singleton pattern with lazy init, we probably need to implement double checked locking or something similar to prevent concurrency issues.
@rhamzeh please chime in with your thoughts.
I agree, I will take a look at this and implement it. Along with maintaining config override for now. |
@jimmyjames We can go for this instead https://www.baeldung.com/java-singleton-double-checked-locking#2-initialization-on-demand |
|
I went through this section Item 83 pg#354 https://kea.nu/files/textbooks/new/Effective%20Java%20%282017%2C%20Addison-Wesley%29.pdf |
SoulPancake
force-pushed
the
ab/otel-pre-init
branch
from
1826f80 to
7def860
Compare
SoulPancake
force-pushed
the
ab/otel-pre-init
branch
from
1826f80 to
60d095d
Compare
SOLVES #209
Description
Implemented a proper singleton pattern by:
Added shared Telemetry instance to Configuration: Added getTelemetry() method with lazy initialization to Configuration class, ensuring one Telemetry instance per configuration.
Updated component constructors: Modified OpenFgaApi, HttpRequestAttempt, and OAuth2Client to use the shared Telemetry instance from Configuration instead of creating their own.
Maintained backward compatibility: Added constructor overloads to HttpRequestAttempt and OAuth2Client to accept Telemetry as a parameter while keeping existing constructors functional.
Updated all call sites: Modified all instantiations of these classes throughout OpenFgaApi to pass the shared Telemetry instance.#### What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit