[CHAT-5063] feat: sdk proxy wrapper for agent tracking #1060
Conversation
WalkthroughThis pull request enhances the client architecture to support the creation of derived or proxy clients. New constructors and methods are added across several classes (including AblyRest, AblyRealtime, AblyBase, HttpCore, and AsyncHttpScheduler) to accept and apply Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as "Client Code"
participant AblyRest as "AblyRest"
participant Base as "Superclass Constructor"
Caller->>AblyRest: createDerivedClient(derivedOptions)
AblyRest->>AblyRest: Invoke protected constructor(underlyingClient, derivedOptions)
AblyRest->>Base: Initialize base properties
AblyRest-->>Caller: Return new AblyRest instance
sequenceDiagram
participant Http as "Http"
participant AsyncSched as "AsyncHttpScheduler"
participant NewHttp as "New Http Instance"
Http->>AsyncSched: exchangeHttpCore(HttpCore)
AsyncSched->>NewHttp: Create new instance with executor and HttpCore
NewHttp-->>Http: Return new Http instance
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/1060/features
github-actions
bot
temporarily deployed
to
staging/pull/1060/javadoc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
lib/src/main/java/io/ably/lib/http/HttpCore.java (1)
72-73: Consider potential immutability and merging strategies for derivedOptions.
StoringDerivedClientOptionsdirectly in this field is straightforward. However, ifDerivedClientOptionsitself becomes mutable or if you want to apply multiple overrides in the future, you may need to add checks or merge logic.lib/src/main/java/io/ably/lib/util/AgentHeaderCreator.java (1)
19-42: Consider adding validation for agent names and versions.While the implementation correctly handles derived agents, consider adding validation to ensure agent names and versions follow a consistent format.
public static String create(Map<String, String> additionalAgents, PlatformAgentProvider platformAgentProvider, DerivedClientOptions derivedOptions) { StringBuilder agentStringBuilder = new StringBuilder(); agentStringBuilder.append(Defaults.ABLY_AGENT_VERSION); if (additionalAgents != null && !additionalAgents.isEmpty()) { agentStringBuilder.append(AGENT_ENTRY_SEPARATOR); agentStringBuilder.append(getAdditionalAgentEntries(additionalAgents)); } String platformAgent = platformAgentProvider.createPlatformAgent(); if (platformAgent != null) { agentStringBuilder.append(AGENT_ENTRY_SEPARATOR); agentStringBuilder.append(platformAgent); } if (derivedOptions != null) { + // Validate agent names and versions + for (Map.Entry<String, String> entry : derivedOptions.getAgents().entrySet()) { + if (!isValidAgentName(entry.getKey()) || !isValidAgentVersion(entry.getValue())) { + throw new IllegalArgumentException("Invalid agent name or version format"); + } + } derivedOptions.getAgents().entrySet().forEach(entry -> { agentStringBuilder.append(AGENT_ENTRY_SEPARATOR); agentStringBuilder.append(entry.getKey()); agentStringBuilder.append(AGENT_DIVIDER); agentStringBuilder.append(entry.getValue()); }); } return agentStringBuilder.toString(); } +private static boolean isValidAgentName(String name) { + return name != null && name.matches("^[a-zA-Z0-9-]+$"); +} + +private static boolean isValidAgentVersion(String version) { + return version != null && version.matches("^[a-zA-Z0-9.-]+$"); +}lib/src/main/java/io/ably/lib/http/Http.java (1)
24-31: Add null check for httpCore parameter.While the implementation is correct, consider adding parameter validation.
public Http exchangeHttpCore(HttpCore httpCore) { + if (httpCore == null) { + throw new IllegalArgumentException("httpCore cannot be null"); + } return new Http(asyncHttp.exchangeHttpCore(httpCore), new SyncHttpScheduler(httpCore)); }android/src/main/java/io/ably/lib/rest/AblyRest.java (1)
42-44: Add parameter validation in the constructor.Consider adding null checks for the parameters.
protected AblyRest(AblyRest underlyingClient, DerivedClientOptions derivedOptions) { + if (underlyingClient == null) { + throw new IllegalArgumentException("underlyingClient cannot be null"); + } + if (derivedOptions == null) { + throw new IllegalArgumentException("derivedOptions cannot be null"); + } super(underlyingClient, derivedOptions); }lib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.java (1)
204-215: Consider adding timeout parameter.The retry logic is good, but consider making the timeout configurable rather than hardcoding 10 retries with 100ms delay.
- private Map<String, String> tryGetServerRequestHeaders() throws InterruptedException { + private Map<String, String> tryGetServerRequestHeaders(int maxRetries, long retryDelayMs) throws InterruptedException { Map<String, String> requestHeaders = null; - for (int i = 0; requestHeaders == null && i < 10; i++) { - Thread.sleep(100); + for (int i = 0; requestHeaders == null && i < maxRetries; i++) { + Thread.sleep(retryDelayMs); requestHeaders = server.getRequestHeaders(); }lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java (1)
131-138: Consider improving method documentation.While the implementation is correct, the documentation could be more descriptive about the purpose and usage of derived clients.
/** - * [Internal Method] - * <p/> - * We use this method to implement proxy Realtime / Rest clients that add additional data to the underlying client. + * Creates a derived client that shares the underlying connection and channels with this client + * while allowing for additional agent information to be included in requests. + * <p> + * This is primarily used for implementing wrapper SDKs (like Chat or Asset Tracking) + * that need to identify themselves in requests while using the core Ably client. + * + * @param derivedOptions Options containing additional agent information + * @return A new AblyRealtime instance that shares the connection with this client */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
android/src/main/java/io/ably/lib/rest/AblyRest.java(2 hunks)java/src/main/java/io/ably/lib/rest/AblyRest.java(1 hunks)lib/src/main/java/io/ably/lib/http/AsyncHttpScheduler.java(1 hunks)lib/src/main/java/io/ably/lib/http/Http.java(1 hunks)lib/src/main/java/io/ably/lib/http/HttpCore.java(5 hunks)lib/src/main/java/io/ably/lib/http/HttpScheduler.java(1 hunks)lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java(3 hunks)lib/src/main/java/io/ably/lib/rest/AblyBase.java(1 hunks)lib/src/main/java/io/ably/lib/rest/DerivedClientOptions.java(1 hunks)lib/src/main/java/io/ably/lib/util/AgentHeaderCreator.java(3 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.java(5 hunks)
👮 Files not reviewed due to content moderation or server errors (4)
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.java
- lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java
- lib/src/main/java/io/ably/lib/http/HttpScheduler.java
- lib/src/main/java/io/ably/lib/rest/AblyBase.java
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime-okhttp
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (29)
lib/src/main/java/io/ably/lib/http/HttpCore.java (4)
13-13: Import is correct and consistent with usage.
The new import forDerivedClientOptionsis appropriately placed and follows standard conventions.
109-119: Private constructor effectively creates a new instance with derived options.
This design cleanly reuses existingHttpCorestate while selectively overridingderivedOptions. Review whether you want to handle null checks onderivedOptionsor potentially merge with existing ones for advanced use cases. Otherwise, looks good.
325-325: Agent header enhanced to include derived agent data.
PassingderivedOptionsintoAgentHeaderCreatorextends the header composition to incorporate proxy or wrapper agents, aligning with the PR goal of better tracking.
473-480: Method to derive a new HttpCore instance is well-structured.
TheapplyDerivedOptionsmethod provides a clear API to replicate anHttpCorewith updated agent tracking. Consider clarifying in documentation that changes are not merged but overwritten, and ensure callers are aware they receive a freshHttpCoreinstance.lib/src/main/java/io/ably/lib/rest/DerivedClientOptions.java (1)
1-34: Builder pattern is clearly implemented.
The newDerivedClientOptionsclass nicely encapsulates additional agent properties. The final map and builder approach promote readability and consistency. If deeper immutability is desired, consider wrapping the map in an unmodifiable collection within the constructor. Otherwise, this is a good addition.java/src/main/java/io/ably/lib/rest/AblyRest.java (2)
36-41: Protected constructor for derived clients adds straightforward extensibility.
Delegating tosuper(underlyingClient, derivedOptions)is consistent with the base class design. For completeness, handle null checks forderivedOptionsif the codebase typically guards against null or merges them if both underlying and new agents need combining.
43-50: Method creates a simpler API for derived REST clients.
createDerivedClientclearly returns a newAblyRestinstance with the providedDerivedClientOptions, facilitating the intended proxy/wrapper pattern. Consider adding small usage examples or references in Javadoc for discoverability.lib/src/main/java/io/ably/lib/http/AsyncHttpScheduler.java (2)
19-21: LGTM! Well-encapsulated constructor.The private constructor properly maintains encapsulation while allowing flexibility in executor initialization.
27-34: LGTM! Well-documented internal method.The
exchangeHttpCoremethod is properly documented as internal and maintains the existing executor while allowing HttpCore exchange, which aligns with the PR's objective of supporting proxy clients.lib/src/main/java/io/ably/lib/util/AgentHeaderCreator.java (1)
44-46: LGTM! Good backward compatibility.The overloaded method maintains backward compatibility by defaulting
derivedOptionsto null.android/src/main/java/io/ably/lib/rest/AblyRest.java (1)
66-73: LGTM! Well-documented factory method.The
createDerivedClientmethod is properly documented as internal and follows the factory pattern for creating derived clients.lib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.java (7)
160-202: LGTM! The test method thoroughly verifies derived client headers.The test effectively validates:
- Derived client's agent header contains both library and derived agent information
- Original client's agent header remains unchanged
- Connection and channels are shared between original and derived clients
204-215: LGTM! The helper method implements a robust retry mechanism.The
tryGetServerRequestHeadersmethod follows the same pattern as the existing request parameters retry logic, ensuring consistent behavior.
217-250: LGTM! The server handler changes properly support header tracking.The changes to
SessionHandlerNanoHTTPDappropriately capture and expose request headers.
160-202: LGTM! Comprehensive test coverage for derived client headers.The test thoroughly verifies:
- Correct agent header format for derived client
- Original client headers remain unchanged
- Connection and channel sharing between original and derived clients
219-249: LGTM! Clean implementation of request header tracking.The changes to
SessionHandlerNanoHTTPDproperly handle request header tracking with clear methods for resetting and retrieving headers.
160-202: LGTM! Comprehensive test coverage for derived client headers.The test verifies:
- Derived client headers include both library version and derived agent info
- Original client headers remain unchanged
- Connection and channels are shared between clients
204-215: LGTM! Well-structured helper method.The retry mechanism is consistent with existing parameter retrieval logic.
lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java (5)
87-94: LGTM! The constructor properly initializes derived clients.The constructor correctly reuses components from the underlying client while applying derived options.
131-138: LGTM! The method provides a clean API for creating derived clients.The method follows the builder pattern and provides a clear interface for creating derived clients.
87-94: LGTM! Well-designed proxy constructor.The package-private constructor correctly reuses the underlying client's components while allowing for proxy creation.
87-94: LGTM! Well-designed constructor for proxy clients.The constructor correctly:
- Calls super with underlying client and options
- Shares channels and connection with underlying client
131-138: LGTM! Clean implementation of derived client creation.The method provides a clear public API for creating derived clients.
lib/src/main/java/io/ably/lib/http/HttpScheduler.java (3)
443-443: LGTM! The visibility change enhances extensibility.Making the
executorfield protected allows subclasses to access it, which is necessary for implementing custom scheduling behavior.
443-443: LGTM! Appropriate visibility change.Changing the executor field to protected improves extensibility while maintaining proper encapsulation.
443-443: LGTM! Appropriate visibility change.Making
executorprotected enables subclasses to access it, improving extensibility.lib/src/main/java/io/ably/lib/rest/AblyBase.java (3)
117-129: LGTM! The constructor properly initializes derived base clients.The constructor correctly:
- Reuses components from the underlying client
- Applies derived options to the HTTP core
- Updates the HTTP instance with the new HTTP core
117-129: LGTM! Well-structured base constructor for proxy clients.The protected constructor properly initializes all components by reusing the underlying client's instances while applying the derived options to the HTTP core.
117-129: LGTM! Well-implemented base constructor for proxy clients.The constructor correctly:
- Copies all fields from underlying client
- Applies derived options to httpCore
- Updates http with new httpCore
We want to be able to distinguish requests made inside the wrapper from those made by the core PubSub SDK without the wrapper, allowing us to track agents across wrapper SDKs such as the Chat SDK or Asset Tracking. To achieve this, we introduce special proxy Realtime and REST clients that inject additional `DerivedClientOptions` parameters into the underlying SDK.
ttypic
force-pushed
the
CHAT-5063/add-sdk-wrapper
branch
from
b5633fd to
a06e5ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lib/src/main/java/io/ably/lib/http/AsyncHttpScheduler.java (1)
27-34: Consider stronger access control for internal method.The implementation is clean and well-documented, perfectly supporting the PR's objective of implementing proxy clients. However, since this is marked as an internal method but exposed as public, consider:
- Using the
@Internalannotation if available in your codebase- Moving this to a separate internal package when Java modules are adopted
lib/src/main/java/io/ably/lib/http/HttpCore.java (2)
72-72: LGTM! Consider adding Javadoc.The member variable is correctly declared as private, but adding Javadoc would improve code documentation.
+ /** Options for derived clients that add additional data to the underlying client. */ private DerivedClientOptions derivedOptions;
473-480: Add nullability annotation and parameter validation.While the implementation is correct, the method could benefit from additional safety checks.
/** * [Internal Method] * <p> * We use this method to implement proxy Realtime / Rest clients that add additional data to the underlying client. + * @param derivedOptions The options to be applied to the derived client, must not be null + * @return A new HttpCore instance with the derived options applied + * @throws NullPointerException if derivedOptions is null */ + @NonNull public HttpCore applyDerivedOptions(DerivedClientOptions derivedOptions) { + Objects.requireNonNull(derivedOptions, "derivedOptions must not be null"); return new HttpCore(this, derivedOptions); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
android/src/main/java/io/ably/lib/rest/AblyRest.java(2 hunks)java/src/main/java/io/ably/lib/rest/AblyRest.java(1 hunks)lib/src/main/java/io/ably/lib/http/AsyncHttpScheduler.java(1 hunks)lib/src/main/java/io/ably/lib/http/Http.java(1 hunks)lib/src/main/java/io/ably/lib/http/HttpCore.java(5 hunks)lib/src/main/java/io/ably/lib/http/HttpScheduler.java(1 hunks)lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java(3 hunks)lib/src/main/java/io/ably/lib/rest/AblyBase.java(1 hunks)lib/src/main/java/io/ably/lib/rest/DerivedClientOptions.java(1 hunks)lib/src/main/java/io/ably/lib/util/AgentHeaderCreator.java(3 hunks)lib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.java(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- lib/src/main/java/io/ably/lib/http/Http.java
- lib/src/main/java/io/ably/lib/http/HttpScheduler.java
- lib/src/main/java/io/ably/lib/rest/AblyBase.java
- java/src/main/java/io/ably/lib/rest/AblyRest.java
- lib/src/main/java/io/ably/lib/util/AgentHeaderCreator.java
- lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java
- lib/src/main/java/io/ably/lib/rest/DerivedClientOptions.java
- android/src/main/java/io/ably/lib/rest/AblyRest.java
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check (29)
- GitHub Check: check-rest-okhttp
- GitHub Check: check (24)
- GitHub Check: check-realtime
- GitHub Check: check (21)
- GitHub Check: check (19)
- GitHub Check: check-rest
🔇 Additional comments (6)
lib/src/main/java/io/ably/lib/http/AsyncHttpScheduler.java (1)
19-21: LGTM! Well-designed constructor for internal use.The private constructor is well-implemented, allowing for executor reuse while maintaining encapsulation.
lib/src/main/java/io/ably/lib/http/HttpCore.java (2)
109-119: LGTM! The copy constructor correctly maintains immutability.The private copy constructor properly copies all fields from the underlying instance and sets the new derived options. The implementation follows good practices by:
- Being marked as private to control instantiation
- Maintaining immutability by copying all fields
- Not performing any complex logic that could throw exceptions
325-325: LGTM! Header generation is properly integrated.The agent header generation is correctly modified to include derived options, maintaining backward compatibility.
lib/src/test/java/io/ably/lib/test/realtime/RealtimeHttpHeaderTest.java (3)
5-5: LGTM!The new imports are appropriate for the added test functionality.
Also applies to: 21-21
204-215: LGTM!The helper method follows the established pattern in the codebase and includes appropriate assertions.
219-219: LGTM!The changes to
SessionHandlerNanoHTTPDfollow the established pattern for handling request parameters.Also applies to: 229-230, 239-241, 247-249
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| public final class DerivedClientOptions { |
There was a problem hiding this comment.
A couple of notes about naming — people didn't like the "derived client" naming on the DR, so in the ably-cocoa PR (ably/ably-cocoa#2014) I settled on the following more explicit naming:
- the options class is called
WrapperSDKProxyOptions - the method is called
createWrapperSDKProxy
There was a problem hiding this comment.
Got it! Do you think it would be less obvious if we removed ‘SDK’ from the name, e.g., WrapperProxyOptions and createWrapperProxy?
There was a problem hiding this comment.
Personally I feel that it's clearer with "SDK"…
|
To check I've understood the scope of this PR — this only addresses adding the wrapper SDK's agents to the |
|
Would you mind explaining to me which HTTP requests will have the wrapper SDK agent added to the |
Yes, I think it's a bit lesser priority for now, but it shouldn't be hard to add |
The agent will be added to all requests initiated by the wrapper instance, including potentially token requests initiated by the wrapper. Personally, I believe the more we cover, the better. I think we should cover billable API calls "at a minimum", I’m not sure it would be a problem if we included all requests. Additionally, distinguishing between billable and non-billable APIs would require extra code, which we would then need to maintain. |
I'm not sure I understand how that works. For example, consider a REST publish. If I've understood correctly, it goes through the ably-java/lib/src/main/java/io/ably/lib/rest/ChannelBase.java Lines 97 to 98 in a06e5ba But here, since we are reusing the |
We want to be able to distinguish requests made inside the wrapper from those made by the core PubSub SDK without the wrapper, allowing us to track agents across wrapper SDKs such as the Chat SDK or Asset Tracking. To achieve this, we introduce special proxy Realtime and REST clients that inject additional
DerivedClientOptionsparameters into the underlying SDK.Summary by CodeRabbit
New Features
Tests