-
Notifications
You must be signed in to change notification settings - Fork 237
feat: Add reservations expiration tracking to DataAPI #1733
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
Conversation
@@ -97,6 +98,10 @@ type ( | |||
// The operatorIds of nonsigners for the batch. | |||
NonSigners []string | |||
} | |||
Reservation struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also throw reservation symbols in here? Would allow us to monitor total allocated capacity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup I can add that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. How are the metrics meant to be used? I'm assuming this is purely for us internally to add grafana alerts when a reservation is about to expire?
Would we ever want to expose reservations info on the blob explorer? If so then we might as well read all the info exposed per reservation by the contract/subgraph?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First use case is for reservation expiration tracking internally but we can extend the scope to any blob explorer requirements in future PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disperser/cmd/dataapi/flags/flags.go
Outdated
//We need the socket address of the subgraph payments api to pull the subgraph data from. | ||
Usage: "the socket address of the subgraph payments api", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this really a socket address? aka ip:port? Or is it a full URL with http endpoint mapping to specific satsuma resource?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a full URL of an http endpoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -97,6 +98,10 @@ type ( | |||
// The operatorIds of nonsigners for the batch. | |||
NonSigners []string | |||
} | |||
Reservation struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. How are the metrics meant to be used? I'm assuming this is purely for us internally to add grafana alerts when a reservation is about to expire?
Would we ever want to expose reservations info on the blob explorer? If so then we might as well read all the info exposed per reservation by the contract/subgraph?
@@ -77,7 +77,7 @@ func RunScan(ctx *cli.Context) error { | |||
if chainState == nil { | |||
return errors.New("failed to create chain state") | |||
} | |||
subgraphApi := subgraph.NewApi(config.SubgraphEndpoint, config.SubgraphEndpoint) | |||
subgraphApi := subgraph.NewApi(config.SubgraphEndpoint, config.SubgraphEndpoint, config.SubgraphEndpoint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of shows that the NewApi config/constructor is generic for no reasons. Are there ever cases where the different subgraphs don't use the same endpoint? This looks weird that we're just passing the same endpoint 3 times. If we don't ever need the different subgraphs to have different endpoints, then prob best to consolidate them into a single endpoint field like in this config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was weird when I saw it but seems like this specific ejection tool thing only needs the middle one but this object needs all three. Open to suggestions on refactoring but feel like it's gonna be a big change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I see. Fine with just adding a comment then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea the ejection tool is my work. Passing duplicate urls is a hack since NewApi
is an abstraction over multiple subgraph clients (ejection only needs one) and I didn't want to justify yak shaving a refactor of 2yo code assumptions built for dependency injection because some esoteric ejection reporting tool needed to call a subgraph 💀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm totally fine with these decisions, just think they should be documented in the code itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3545ebd
to
d5efc06
Compare
disperser/cmd/dataapi/flags/flags.go
Outdated
//We need the socket address of the subgraph payments api to pull the subgraph data from. | ||
Usage: "the socket address of the subgraph payments api", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -77,7 +77,7 @@ func RunScan(ctx *cli.Context) error { | |||
if chainState == nil { | |||
return errors.New("failed to create chain state") | |||
} | |||
subgraphApi := subgraph.NewApi(config.SubgraphEndpoint, config.SubgraphEndpoint) | |||
subgraphApi := subgraph.NewApi(config.SubgraphEndpoint, config.SubgraphEndpoint, config.SubgraphEndpoint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea the ejection tool is my work. Passing duplicate urls is a hack since NewApi
is an abstraction over multiple subgraph clients (ejection only needs one) and I didn't want to justify yak shaving a refactor of 2yo code assumptions built for dependency injection because some esoteric ejection reporting tool needed to call a subgraph 💀
* Initialize subgraph * Initialize subgraph * Initialize subgraph * Save payment subgraph state * fix * Add reservation collector to dataapi * Add query examples + more test coverage * Remove docker compose * Add scripts for updating abi and adds a CI check when subgraph tests run * fix flags * fix lint error * Fix rebase issues * Inject the registry * wrap errors * expose more info from reservation data * fmt * rename updateCounts to updateMetrics * Document subgraph.NewApi in ejections tool
* Initialize subgraph * Initialize subgraph * Initialize subgraph * Save payment subgraph state * fix * Add reservation collector to dataapi * Add query examples + more test coverage * Remove docker compose * Add scripts for updating abi and adds a CI check when subgraph tests run * fix flags * fix lint error * Fix rebase issues * Inject the registry * wrap errors * expose more info from reservation data * fmt * rename updateCounts to updateMetrics * Document subgraph.NewApi in ejections tool
Why are these changes needed?
Will change base branch to master after #1709 is merged.
Reservation expiration tracking queries a subgraph to get the latests reservations for all accounts. The metrics are updated on every scrape to the
/metrics
endpoint.Sample output on Sepolia
Checks