ably / specification

The Ably features spec for client library SDKs.
Apache License 2.0
0 stars 4 forks source link

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

Open lawrence-forooghian opened 2 months ago

lawrence-forooghian commented 2 months ago

Background

This arose in the context of https://github.com/ably-labs/ably-chat-swift/issues/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:

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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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:

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

Things I’m not sure of re current behaviour

Assumptions

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

Proposal

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 commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

Internal RFC currently in review for this.