feat: add account/egress/get capability - #77
Conversation
alanshaw
left a comment
alanshaw
left a comment
There was a problem hiding this comment.
No blocking feedback but would be great to get some comments here to explain how it works/is intended to be used. I guess a PR to specs would have the same effect.
|
|
||
| type Period struct { | ||
| From time.Time | ||
| To time.Time |
There was a problem hiding this comment.
Could use a comment. What kind of resolution are we allowed here, if I specify "to" as midday, will I only get egress registered before midday on that day?
There was a problem hiding this comment.
From inclusive and to exclusive right?
There was a problem hiding this comment.
Could use a comment. What kind of resolution are we allowed here, if I specify "to" as midday, will I only get egress registered before midday on that day?
you'd get egress for the whole day. The focus is on daily stats, so only the date part of the time.Time is used.
From inclusive and to exclusive right?
the etracker will do a BETWEEN query to the dynamoDB table, so yes, both inclusive.
There was a problem hiding this comment.
sorry, apparently I cannot read. My initial idea was for both of them to be inclusive, but I see from is inclusive and to is exclusive in account/usage/get, so I guess it's better to keep it consistent here.
| } | ||
|
|
||
| type SpaceEgress struct { | ||
| Total uint64 |
There was a problem hiding this comment.
Could use a comment - is this just the sum of all the egress values in DailyStats?
|
|
||
| type SpaceEgress struct { | ||
| Total uint64 | ||
| DailyStats []DailyStats |
There was a problem hiding this comment.
Are these ordered by date ascending? Would be good to know.
There was a problem hiding this comment.
the date is the sort key in the table where the etracker stores these stats, so yes
| } | ||
|
|
||
| type GetOk struct { | ||
| Total uint64 |
There was a problem hiding this comment.
The total egress across all spaces for the period?
|
|
||
| type SpaceEgress struct { | ||
| Total uint64 | ||
| DailyStats []DailyStats |
There was a problem hiding this comment.
Might it be worth calling this something more generic to allow breaking down in smaller increments in the future. Like right now you want daily, but it might be desirable to break down into 10 minute intervals or something for a "zoomed in" view.
There was a problem hiding this comment.
that's an interesting idea. These could be just Stats and we could add an additional caveat to allow asking for different resolutions.
I think it's preferable to keep it simple for now and we can improve it if/when the need arises. I'm not sure right now whether resolutions smaller than a day would be useful for customers.
| var GetCaveatsReader = schema.Struct[GetCaveats](GetCaveatsType(), nil, types.Converters...) | ||
|
|
||
| type DailyStats struct { | ||
| Date time.Time |
There was a problem hiding this comment.
To and From to allow specifying a period that is less than day resolution?
There was a problem hiding this comment.
again, I'd prefer keeping it as is for now and see if the extra flexibility is actually needed in the future
| } | ||
|
|
||
| type GetCaveats struct { | ||
| Spaces []did.DID |
There was a problem hiding this comment.
I assume you can specify no spaces here to get egress for them all? Maybe add a comment.
| Message string | ||
| } | ||
|
|
||
| const AccountNotFoundErrorName = "AccountNotFoundError" |
There was a problem hiding this comment.
Do we need an error for if you ask for egress for space(s) you do not have access to?
Also a range error for when you ask for a period that is bigger than what we want to allow?
There was a problem hiding this comment.
Do we need an error for if you ask for egress for space(s) you do not have access to?
if there are no stats for the space, you'll get the space in the map with 0 egress. If you don't have access to the space, you won't have the space in the map at all. That would be enough for the client to know what happened. But I like being explicit.
What should happen if you request several spaces and have access to some of them but not all? Should we fail the whole request? I think ignoring non-authorized spaces could be a better approach. At the same time, if none of the requested spaces is accessible, returning an ok result with an empty spaces map sounds a bit weird to me.
I think ignoring non-authorized spaces works here because the resource of the capability is the account, so we will expose spaces that were created by that account, and not those the account has access to.
WDYT?
Also a range error for when you ask for a period that is bigger than what we want to allow?
👍🏻
There was a problem hiding this comment.
about the spaces thing, I'll keep it consistent with the behavior of account/usage/get, which fails the whole request when any of the requested spaces is not authorized.
|
Great points, thank you. I wrote the plan in the RFC and forgot about spec'ing the capability. I opened a PR to specs and linked it in the description. I also added comments here and some more errors. |
Ref. storacha/project-tracking#619 📚 [Preview](https://github.com/storacha/specs/blob/8947e4034ec492ce95c6b42df393d625677d7ede/w3-account.md#accountegressget) Add the `account/egress/get` capability specification. The first use of this capability will be in Storacha Forge's customer dashboard, described in https://github.com/storacha/RFC/blob/cce367c20afaf9f8f8be799b0da5061e0bdd4898/rfc/billing-dashboard.md#customer-dashboard. Implemented in storacha/go-libstoracha#77 and storacha/upload-service#638. --------- Co-authored-by: bravonatalie <natalie.bravo@outlook.com>
2 participants
Ref. storacha/project-tracking#619
Spec: storacha/specs#149
Add a new capability for reporting egress in the forge network. It will be handled by the egress tracking service and is built in resemblance of the upload service's
account/usage/get.The invocation allows requesting specific spaces for a given period. Both caveats are optional. A successful result contains a total egress number, as well as results by space with daily numbers.