-
Notifications
You must be signed in to change notification settings - Fork 2
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 evaluate service #1
Conversation
ieContract.on('MeasurementsAdded', (cid, _roundIndex) => { | ||
const roundIndex = Number(_roundIndex) | ||
if (cidsSeen.includes(cid)) return | ||
cidsSeen.push(cid) |
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.
IIUC, this array will keep growing until we run out of memory.
Have you considered remembering only the last N items, where N is for example 1000?
Also, what happens when we restart the service, e.g. after deploying a new version? The list of CIDs seen will be lost. How is that going to impact the evaluation?
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.
Added a fix 👍
The service restarting isn't a problem, we're mostly debouncing here anyway
- if we preprocess redundantly, there will be extra work but the results won't change
- if we evaluate redundantly, there will be extra work and gas, but the results won't 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.
Nice! I have a few minor comments to discuss.
self heal / discover all measurements that haven't been preprocessed yet, and all rounds that haven't been evaluated yet.
This can also be added in a new PR
Yeah, let's do that after this initial PR is landed.
const res = await web3Storage.get(cid) | ||
const files = await res.files() | ||
const measurements = JSON.parse(await files[0].text()) | ||
return measurements |
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.
Observability (after this PR is landed): I would consider reporting the number of measurements as a new telemetry point and visualising it in Grafana. I think both the "number of measurements per CID" and "number of measurements per round" are potentially useful metrics to watch.
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.
sounds good!
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.
LGTM as the first incremental version 👏🏻
I consider my three comments above about observability and error handling as still relevant, but they can be addressed in follow-up pull requests.
This has already been deployed to fly.io 🙏
Closes CheckerNetwork/roadmap#29
Closes CheckerNetwork/roadmap#33
@bajtos happy to fulfil change requests or hand over
TODO: