Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a way to communicate agent information using an existing Realtime instance #201

Open
lawrence-forooghian opened this issue Aug 19, 2024 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@lawrence-forooghian
Copy link
Collaborator

lawrence-forooghian commented Aug 19, 2024

Background

This arose in the context of ably-labs/ably-chat-swift#18.

We currently have an agents client option (RFC7d6) but when this was created our non-core SDKs (e.g. Asset Tracking) were following a pattern in which said SDK would itself instantiate a RealtimeClient object, populating the agents client option with information about said SDK. These days, we follow (e.g. in Spaces and Chat) a different pattern in which the user of the SDK instantiates a RealtimeClient object.

The JS Chat SDK has "solved" this problem by using private API to access the private options property of the realtime client and then mutating these options’ agents property. For the Swift and Kotlin Chat SDKs, I would like us to instead be able to use an API that’s properly specified in this spec, even if marked as "experimental and for Ably-authored SDKs" only.

Requirements

From this internal Slack conversation with @AndyTWF, it sounds like the key requirement is that there be a way to instruct a given RealtimeClient instance to append a value to the Ably-Agent header that is sent when the client makes REST requests via the #request method.

Non-requirements?

From the aforementioned conversation with Andy, it sounds like the following are not needed at least for Chat:

  • the ability to control the Ably-Agent header that’s sent when a WebSocket connection is established to Ably (for tracking realtime Chat usage, it sounds like channel params are used instead)

but we need to confirm this with whoever is making use of this agent information (@ruimffl I think?)

Some things that might work for now

  1. A client SDK could instead form the Ably-Agent header itself, using ClientInformation.agentIdentifier (CR3), and then pass this header in #request’s headers param. We’d need to make sure that we specified that a user-specified Ably-Agent header overrides that added by the SDK.
  2. Or, we could add an agents argument to #request.

Scope of the mutation

Ideally, the above would actually read "when the client makes certain REST requests via the #request method"; that is, we don’t want to add this header to requests that the user issues through the RealtimeClient object.

┆Issue is synchronized with this Jira Task by Unito

@jamienewcomb
Copy link
Member

@lawrence-forooghian @umair-ably @ttypic - how do we move this forward? could one of you own putting together a proposal around how we will address this? once we have this agreement we can then open the tickets in the relevant SDKs

I have created https://ably.atlassian.net/browse/ECO-4958 which one of you can assign yourselves to

@ttypic
Copy link
Contributor

ttypic commented Aug 29, 2024

Thanks @jamienewcomb! @lawrence-forooghian and @umair-ably it will be great if one of you can pick up this later, we can discuss this on the next planning, I added my thoughts to the jira ticket comments

@lawrence-forooghian lawrence-forooghian self-assigned this Sep 6, 2024
@lawrence-forooghian
Copy link
Collaborator Author

Adding @ttypic’s comment here to keep all discussion in this issue:

We need to clarify the product requirements; we probably need to get direction from Rui and maybe Realtime team first. If we make changes to the spec, it should be a universal solution, not just for Chat. We already have a workaround in Spaces, another in Chat, and some magic calculations to support this on the analytics side. I personally have a very vague understanding of how stats are calculated.

For REST requests, the problem seems fairly trivial. We probably can always use the agent parameter, as we do for Chat and Spaces in JS. However, the current approach isn’t ideal—we're mutating RealtimeClient to add the agent, which is bad practice. Additionally, this likely introduces incorrect business logic, as the newly added agent will be propagated to all requests, not just those made by the Chat SDK.

For the Realtime connection, it’s a bit more complicated. The connection can be established either before or after the ChatClient (Chat here is an example) is created, so we can’t rely on the connection's agent parameter. The best option might be to use special channel parameters, but I don't think we currently have anything universal (like the agent parameter) that isn’t specific to Chat or Spaces for this purpose.

@lawrence-forooghian
Copy link
Collaborator Author

lawrence-forooghian commented Sep 9, 2024

I’ve looked into it some more, and it seems like the Chat SDK’s approach to analytics is a copy of that of the Spaces SDK. That is, these wrapper SDKs:

  • Use ably-js private API to add an agent describing the wrapper SDK to the client options of the Realtime client that they were instantiated with; this has the effect of:
    • including the agent of the wrapper SDK in all REST requests made by the Realtime client subsequent to the initialisation of the wrapper SDK
    • including the agent of the wrapper SDK in the connection params for all realtime transport connections made by the Realtime client subsequent to the initialisation of the wrapper SDK (which, in the most common case, will mean on all realtime reconnections but not the initial connection)
  • Pass the agent channel param (e.g. spaces/1.2.3) to channels.get; this has the effect of sending the agent of the wrapper SDK in the ATTACH protocol message for that channel

The decision records that led to this behaviour seem to be:

Things I’m not sure of re current behaviour

  • Do we want to attribute realtime connection events to any wrapper SDK? Doesn't seem right given that a realtime connection can multiplex traffic from multiple wrapper SDKs, and also from non-wrapper usage of the same Realtime client. However, MMDR2 does list a couple of requirements that sound like they were intended to be implemented by attributing connections to a wrapper SDK:

    • No. of instantiations of the Space API
    • No. of peak connections set up via Space API methods (and total connections if possible)
  • Do we want to attribute all REST requests made by a given Realtime client to a wrapper SDK? I’d have thought that we only wish to attribute those that are explicitly made by that wrapper SDK.

Assumptions

I’ll assume the following, but need to be corrected if there are essential product requirements that these assumptions violate:

  • The only things we wish to be able to attribute to a given wrapper SDK are:

    • REST requests
    • Channel attach events

    In particular, we do not wish to attribute a realtime connection event to any wrapper SDK.

Proposal

  • For attributing REST requests to a wrapper SDK, add an optional agents argument to {RestClient, RealtimeClient}#request, which accepts a ClientOptions.agents-like set of key-value pairs which will be added to the RSC7d2 Ably-Agent header for that request. Document that this argument is only intended for Ably-authored SDKs, the same way as we do for ClientOptions.agents.
  • For attributing channel attach events to a wrapper SDK, continue to use the Spaces / JS Chat approach of using the agent channel param.

If people are happy with this approach, then I’ll also create an SDK decision record in order to record this as our decided approach to analytics for wrapper SDKs.

@lmars
Copy link
Member

lmars commented Sep 9, 2024

Do we want to attribute realtime connection events to any wrapper SDK? Doesn't seem right given that a realtime connection can multiplex traffic from multiple wrapper SDKs, and also from non-wrapper usage of the same Realtime client

I don't think this was the intention, no.

I think the original requirements of being able to attribute connections to the Spaces SDK were from before we changed the API so that Spaces didn't initialise its own client and thus no longer "owned" the connection, which is why we subsequently added the agent channel param so that we could attribute attachments to the SDK, and then indirectly the connections those attachments belong to.

Do we want to attribute all REST requests made by a given Realtime client to a wrapper SDK?

I don't think the Spaces SDK should be mutating the agents field on the client options, although it should be able to add the agent to REST requests it is responsible for initiating.

@AndyTWF
Copy link
Contributor

AndyTWF commented Sep 10, 2024

Do we want to attribute all REST requests made by a given Realtime client to a wrapper SDK?

I'm with Lewis on this one. For Chat we just followed what Spaces did, but from a data quality point of view, we only want requests actually made by Chat to show up as such.

So a parameter to that effect on the REST request method seems like a good way to go.

@lawrence-forooghian
Copy link
Collaborator Author

Internal RFC currently in review for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

5 participants