Add API for tracking usage of wrapper SDKs #273
Conversation
lawrence-forooghian
requested review from
VeskeR,
ttypic,
sacOO7,
maratal and
umair-ably
|
Ah, need to add docstrings, will do that now. |
lawrence-forooghian
force-pushed
the
wrapper-sdk-proxy
branch
from
f97a7cf to
b53e493
Compare
This specifies the API agreed in ADR-117 [1], to allow us to track usage of a "modern wrapper SDK" (the term used in that ADR to refer to a wrapper SDK which accepts an already-instantiated core SDK client). We will use it to track usage of the Chat SDK. The functionality described here has already been partially implemented in ably-cocoa [2] and is being used by ably-chat-swift [3]. I found it quite hard to write a spec for this, and the language might still be both too vague and too convoluted. I'm putting this up for review as a first attempt in order not to end up getting stuck trying to write something perfect. [1] https://ably.atlassian.net/wiki/spaces/ENG/pages/3413016589/ADR-117+Tracking+wrapper+SDK+usage+continued [2] ably/ably-cocoa#2014 [3] ably/ably-chat-swift#211
lawrence-forooghian
force-pushed
the
wrapper-sdk-proxy
branch
from
b53e493 to
45b9796
Compare
Done. |
| * @(WP4)@ A wrapper SDK proxy client should not provide a @createWrapperSDKProxy@ method. | ||
| * @(WP5)@ A wrapper SDK proxy client does not have any state of its own; rather, it shares all state with its underlying client. | ||
| * @(WP6)@ When the following methods are called via a wrapper SDK proxy client, then the resulting REST request must be attributed to the wrapper SDK (as defined in "@RSC7d7@":#RSC7d7), using the @WrapperSDKProxyOptions@ that were used to create the wrapper SDK proxy client: | ||
| ** @(WP6a)@ @RestChannel#publish@ |
There was a problem hiding this comment.
Do we need a specific list of methods? Would we not want to make it so for any methods that interact with the server?
There was a problem hiding this comment.
The list here is based on Matt saying in ADR-117 that we're interested in tracking billable operations.
I agree that it would be nice to try and write the spec here in a more general fashion, but I'm struggling to think of how we'd express it. Because initially we'd try and say something like "all REST requests initiated via a wrapper SDK proxy should be attributed to that wrapper SDK", but there are some REST requests where the meaning of "initiated via a wrapper SDK proxy" gets a bit muddy when the underlying client is not being used exclusively by a single wrapper SDK, because the requests happen as part of some common process that is difficult to attribute to a single user action, for example token requests or the REST calls involved in registering a device for push notifications. I guess we could explicitly exclude these REST calls from the "all REST requests initiated via a wrapper SDK proxy should be attributed to that wrapper SDK" spec point.
There was a problem hiding this comment.
Gonna have a think about how we can re-word this one (I agree it's not great as is, means that we have to keep making sure that we update this list each time we add a new billable operation REST API).
|
|
||
| A _wrapper SDK_ is an Ably-authored non-core SDK. | ||
|
|
||
| The core SDK provides an API for wrapper SDKs to supply Ably with analytics information that allows us track the usage of these SDKs. |
There was a problem hiding this comment.
Something that we should perhaps consider here - right now, connections to the server are made by the underlying SDK, and because autoconnect=true we never see connections from the wrapper SDKs.
Obviously with autoconnect, that's not something we can realistically do when the option is turned on. But perhaps we should decide if we want to provide wrapper details in connections in any way, and also specify it?
There was a problem hiding this comment.
So, in my assumptions of the requirements in ADR-117 I assumed that we did not want to attribute Realtime connection events to a wrapper SDK. This was based on the assumption that autoconnect would be used, though. But, if you look at Matt's response to my question about what we'd like to track, he seems to believe that connections are not something we are interested in tracking.
There was a problem hiding this comment.
Ah perfect, that's already been discussed :) Yeah I agree attachments are more valuable than just connections.
There was a problem hiding this comment.
I think tracking connections is fundamentally incorrect in terms of analytics. The connection is initiated by RealtimeClient, so it shouldn’t be counted as Chat SDK usage. Otherwise, the wrapper SDK usage will obscure all RealtimeClient usage, leading to incorrect assumptions
There was a problem hiding this comment.
I think tracking connections is fundamentally incorrect in terms of analytics. The connection is initiated by
RealtimeClient, so it shouldn’t be counted as Chat SDK usage. Otherwise, the wrapper SDK usage will obscure allRealtimeClientusage, leading to incorrect assumptions
Yeah exactly, all of our current chat analytics are based on attachments + frontend API requests anyway
| A _wrapper SDK_ is an Ably-authored non-core SDK. | ||
|
|
||
| The core SDK provides an API for wrapper SDKs to supply Ably with analytics information that allows us track the usage of these SDKs. | ||
|
|
There was a problem hiding this comment.
Another thing that comes to mind - in Chat we have a use-case whereby the SDK agents are changed "on the fly" (not part of the constructor). This is in the React case, whereby we set the agents when a client is passed to the React component.
Is this something that's supported here / needs further speccing?
There was a problem hiding this comment.
That's not a use case that I was aware of. We'd probably want to have an API that allows you to mutate the agents of a wrapper SDK proxy client in that case. I don't know whether we should specify it here or implement it as a JS-only API.
There was a problem hiding this comment.
It's probably worth specifying, if only with a comment that says it primarily pertains to React-like circumstances.
There was a problem hiding this comment.
Do we really want to mutate the agent? I’d rather think of a proxy as a decorator (those two GoF patterns are almost identical). Now we have RealtimeClient and WithChatAgent(RealtimeClient), which powers ChatClient.
Can we use WithReactAgent(ChatClient) inside React internals? And what if WithReactAgent(ChatClient) is powered by WithReactAgent(WithChatAgent(RealtimeClient))?
Does that make sense?
There was a problem hiding this comment.
Thinking about this some more, and I'm wondering whether what we could do instead in chat-js is, upon realising that we're operating in a React context, discard the initial wrapper SDK proxy client that we created upon instantiation of the chat client, and call createWrapperSDKProxyClient again to create a new wrapper SDK proxy client which includes the React agent. I assume this would require some changes to the JS chat SDK to allow the client that's being used internally to be switched out. Would this be an option?
There was a problem hiding this comment.
Thinking about this some more, and I'm wondering whether what we could do instead in chat-js is, upon realising that we're operating in a React context, discard the initial wrapper SDK proxy client that we created upon instantiation of the chat client, and call
createWrapperSDKProxyClientagain to create a new wrapper SDK proxy client which includes the React agent. I assume this would require some changes to the JS chat SDK to allow the client that's being used internally to be switched out. Would this be an option?
That could work, right now React calls an underlying method on the chat client to "add the agent". That method could just get a new wrapper instance. As getting a new wrapper instance still wraps the same realtime client, we can just replace the wrapper in the one place that'll need it.
There was a problem hiding this comment.
@ttypic I addressed your composition question in #273 (comment)
There was a problem hiding this comment.
Ah @ttypic I just realised that we replied to Andy at about the same time; #273 (comment) wasn't intended to be a reply to #273 (comment) 🙂
| * @(WP1)@ A _wrapper SDK proxy client_ is a client that is created by calling the @createWrapperSDKProxy@ method on an instance (hereafter referred to as the _underlying client_) of @RestClient@ ("@RSC26@":#RSC26) or @RealtimeClient@ ("@RTC14@":#RTC14). It is expected that a wrapper SDK proxy client will only be created by a wrapper SDK. | ||
| * @(WP2)@ A wrapper SDK proxy client must provide the same public API (that is, methods and properties) as its underlying client. Calling a method or accessing a property of a wrapper SDK proxy client should behave as if the same action had been performed directly on the underlying client, except for some modified usage tracking behaviour which is described in "@WP6@":#WP6 and "@WP7@":#WP7. | ||
| * @(WP3)@ This specification does not prescribe how a wrapper SDK proxy client should be implemented nor its concrete type. An implementation may choose to replace the return value of methods and properties with an implementation-defined type that differs from that returned by the underlying client; for example, a wrapper SDK proxy client's @channels@ property may return an object of some class which wraps the underlying @Channels@ instance and forwards all method calls to it. And, in turn, this type's @#get@ method may return an object of a class which wraps the underlying @RestChannel@ or @RealtimeChannel@ class and forwards all method calls to it. | ||
| * @(WP4)@ A wrapper SDK proxy client should not provide a @createWrapperSDKProxy@ method. |
There was a problem hiding this comment.
Why do you think we should prevent this? I think with React-case it can be useful. Also if you can do this, I believe it shows that implementation has good design
There was a problem hiding this comment.
I think that it could be hard to implement, because — at least in the ably-cocoa implementation — the wrapper SDK proxy client relies on private API of the underlying client; that is, it wouldn't work if the underlying client only conformed to the public interface.
There was a problem hiding this comment.
(I mean, we could make the wrapper SDK proxy client and the public RealtimeClient class conform to some common private interface, but I'm not yet convinced it's worth it)
This specifies the API agreed in ADR-117, to allow us to track usage of a "modern wrapper SDK" (the term used in that ADR to refer to a wrapper SDK which accepts an already-instantiated core SDK client). We will use it to track usage of the Chat SDK.
The functionality described here has already been partially implemented in ably-cocoa (ably/ably-cocoa#2014) and is being used by ably-chat-swift (ably/ably-chat-swift#211).
I found it quite hard to write a spec for this, and the language might still be both too vague and too convoluted. I'm putting this up for review as a first attempt in order not to end up getting stuck trying to write something perfect.