Uh oh!
There was an error while loading. Please reload this page.
This repository was archived by the owner on Aug 10, 2026. It is now read-only.
[EDX-175] Document the stats types and add to the IDL - #1453
Merged
Conversation
lawrence-forooghianforce-pushed
the
EDX-175-document-stats-types-and-add-to-IDL
branch
from
June 9, 2022 14:44
94276c9 to
6a8db15Comparelawrence-forooghian
marked this pull request as ready for review
June 9, 2022 14:46
lawrence-forooghian
requested review from
QuintinWillison, lmars, owenpearson and paddybyersJune 9, 2022 14:46
owenpearson
commented
Jun 13, 2022
Member
Hey @lawrence-forooghian my review for this is still in progress, just posting here for visibility. I don't feel too comfortable with adding a bunch of TODOs to the spec, so would like to get descriptions added for all of the stats fields if possible. I haven't been find any existing descriptions so far (the realtime codebase doesn't have any), but worse case scenario is I'll end up writing some myself. |
My initial motivation here is EDX-175. We want the entire public API of our SDKs to have docstring comments. We will be approaching this by using this spec’s IDL as our source of truth for what constitutes our SDK’s public API, to which tech writers will then add generic descriptive comments. Hence, the IDL will need to contain the stats-related types, which it currently doesn’t. The first step towards achieving this is to make sure that all of these types are documented in the spec. Currently, the types are not documented, instead pointing the reader towards the Ruby documentation. I don’t think it makes sense for a generic feature spec to refer to a specific implementation as a reference - seems a bit circular, and requires the reader to have at least a passing knowledge of that language. Furthermore, it means that there’s a chunk of the feature spec that won’t go through the usual feature spec review process - any changes to that part of the Ruby SDK will become changes to the feature spec, without the Ruby SDK developers necessarily even realising this. Also, at the time of writing, the Ruby SDK generated documentation was broken, not displaying any classes at all (all of the links currently in the feature spec giving 404s). The lack of stats documentation has also caused us to be blocked on ably/ably-flutter#106 (adding stats functionality to the Flutter SDK), because of inconsistencies between ably-cocoa and ably-java which the current documentation was not sufficient to help us resolve. The information and comments that I’ve added here are taken from ably-ruby@f5eac15. I’ve lightly edited the descriptions, but I think they still could be improved a lot. I think I’ll need input from someone with more knowledge of what the numbers in the stats response actually mean, though. There are a couple of types mentioned in the spec which are not actually implemented in the Ruby SDK – PushStats and XchgMessages. I’ll handle those separately.
Motivation as in a20f79a, but as mentioned there this type doesn’t exist in the Ruby SDK. So I’ve looked at an example response and how some of our other SDKs handle this type. The example JSON on /general/statistics has > "push": { // Detailed stats on push notifications, see > // https://ably.com/documentation/general/push for more details > "messages": 0, > "notifications": { > "invalid": 0, > "attempted": 0, > "successful": 0, > "failed": 0 > }, > "directPublishes": 0 > }, The response from the curl command given on that page doesn’t contain any `push` properties. ably-java@7fc1939: > public static class PushedMessages { > public int messages; > public Map<String, Integer> notifications; > public int directPublishes; > } ably-js@cd35d6b: > type NotificationsValues = { > invalid?: number; > attempted?: number; > successful?: number; > failed?: number; > }; > > type PushValues = { > messages?: number; > notifications?: NotificationsValues; > directPublishes?: number; > }; ably-cocoa@0fe3c59: > @interface ARTStatsPushCount : NSObject > > @Property (readonly, assign, nonatomic) NSUInteger succeeded; > @Property (readonly, assign, nonatomic) NSUInteger invalid; > @Property (readonly, assign, nonatomic) NSUInteger attempted; > @Property (readonly, assign, nonatomic) NSUInteger failed; > > @Property (readonly, assign, nonatomic) NSUInteger messages; > @Property (readonly, assign, nonatomic) NSUInteger direct; > > @EnD So, it seems like the ably-js approach is the most consistent with the spec’s current approach of having a new type for each nested object in the JSON. I don’t know what any of the properties I’ve added actually mean, hence the TODOs for descriptions. I’d welcome any input here.
Since this type does not exist in ably-ruby, we take the same approach as 400d057. Same comment applies re TODOs for descriptions. The xchgConsumer/Producer properties are not even mentioned in the example JSON at /general/statistics/. In the response for the example curl command given on that page, xchgProducer is always `null`, and an example of xchgConsumer is: > { > "all": { > "all": { > "count": 143, > "data": 4972906, > "uncompressedData": 7924857, > "category": { > "delta": { > "count": 69, > "data": 871973, > "uncompressedData": 3823924 > } > } > }, > "messages": { > "count": 143, > "data": 4972906, > "uncompressedData": 7924857, > "category": { > "delta": { > "count": 69, > "data": 871973, > "uncompressedData": 3823924 > } > } > }, > "presence": {} > }, > "producerPaid": { > "all": { > "all": { > "count": 143, > "data": 4972906, > "uncompressedData": 7924857, > "category": { > "delta": { > "count": 69, > "data": 871973, > "uncompressedData": 3823924 > } > } > }, > "messages": { > "count": 143, > "data": 4972906, > "uncompressedData": 7924857, > "category": { > "delta": { > "count": 69, > "data": 871973, > "uncompressedData": 3823924 > } > } > }, > "presence": {} > }, > "inbound": null, > "outbound": { > "realtime": { > "all": { > "count": 143, > "data": 4972906, > "uncompressedData": 7924857, > "category": { > "delta": { > "count": 69, > "data": 871973, > "uncompressedData": 3823924 > } > } > }, > "messages": { > "count": 143, > "data": 4972906, > "uncompressedData": 7924857, > "category": { > "delta": { > "count": 69, > "data": 871973, > "uncompressedData": 3823924 > } > } > }, > "presence": {} > }, > "all": { > "all": { > "count": 143, > "data": 4972906, > "uncompressedData": 7924857, > "category": { > "delta": { > "count": 69, > "data": 871973, > "uncompressedData": 3823924 > } > } > }, > "messages": { > "count": 143, > "data": 4972906, > "uncompressedData": 7924857, > "category": { > "delta": { > "count": 69, > "data": 871973, > "uncompressedData": 3823924 > } > } > }, > "presence": {} > } > } > }, > "consumerPaid": null > } We only have two SDKs that have implemented this type (ably-js and ably-rust). ably-js@cd35d6b: > class XchgMessages { > all?: MessageTypes; > producerPaid?: MessageDirections; > consumerPaid?: MessageDirections; > } > > class MessageDirections { > all?: MessageTypes; > inbound?: MessageTraffic; > outbound?: MessageTraffic; > } ably-rust@7385e17: > pub struct XchgMessages { > pub all: MessageTypes, > pub producer_paid: MessageDirections, > pub consumer_paid: MessageDirections, > } > > pub struct MessageDirections { > pub all: MessageTypes, > pub inbound: MessageTraffic, > pub outbound: MessageTraffic, > }
The response from the curl command given at /general/statistics shows
that `mean` can in fact have a fractional component (which makes sense):
> (...)
> "connections": {
> "all": {
> "peak": 11,
> "min": 9,
> "mean": 9.532,
> "opened": 13
> },
> "tls": {
> "peak": 11,
> "min": 9,
> "mean": 9.532,
> "opened": 13
> }
> },
> "channels": {
> "peak": 7,
> "min": 5,
> "mean": 5.828,
> "opened": 3
> },
> (...)These are based on the descriptions in the stats version 2 API schema (https://github.com/ably/ably-common/blob/2d2152ff29bef731b74a81ecce406d8cfbaf0abf/json-schemas/src/app-stats.json#L767-L796) and the mapping between the version 2 and version 1 schemas (https://github.com/ably/realtime/blob/485fb8cd03ad6c181cdf25bd7586cf7aa7543307/common/lib/types/stats.ts#L378-L386). That’s not enough to cover all the fields, though.
This is my _guess_ as to what the fields mean, based on reading ably/ideas#264 (comment) and the API Streamer documentation. Needs an eye from someone who can confirm.
lawrence-forooghianforce-pushed
the
EDX-175-document-stats-types-and-add-to-IDL
branch
from
June 13, 2022 19:56
6a8db15 to
fd1ec09Comparelawrence-forooghian
commented
Jun 13, 2022
ContributorAuthor
@owenpearson I've had a go at writing some, based on pretty shaky sources of information / my guesses (see the commit messages for more details). There's also a couple left that need documenting. Do you have any thoughts on how we might be able to move this forwards now? |
owenpearson
commented
Jun 17, 2022
Member
@lawrence-forooghian I've looked at the realtime code and have come up with these suggestions to fill in the gaps:
|
owenpearson
suggested changes
Jun 17, 2022
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
lawrence-forooghian
commented
Jun 19, 2022
ContributorAuthor
Thanks @owenpearson for your help here! I'm going to hold off on making any further changes here until I've got a better idea on whether we want to proceed with documenting the stats types (internal Slack discussion). |
I had kept it as it was in the source of my copy-and-pastes in ade93bb, but code review said to change it, which makes sense.
Owen: > I've looked at the realtime code and have come up with these > suggestions to fill in the gaps:
lawrence-forooghian
commented
Jun 29, 2022
ContributorAuthor
@owenpearson I've added your suggestions. Would you mind taking another look at this one, please? |
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This adds documentation for all the stats-related types, instead of relying on links to the Ruby documentation.
Review
Please review commit-by-commit and see commit messages for motivation and approach.
Outstanding
If anybody would like to suggest descriptions for the things that say
TODO description, I'd be very grateful. Else I'll leave the TODOs as is to be addressed some other time and will make an issue for it.