Skip to content
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 error reporting to progress report API call #44

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

vegarsti
Copy link
Member

@vegarsti vegarsti commented Jun 28, 2024

This PR makes it so we collect errors and send them to Dune as part of the progress report request.

I added two models in this PR, one in the models package and one in the DuneAPI package. This means we have three structs for representing the same kind of error (there's also one in `ingester). Is that too much? 😅

Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @vegarsti and the rest of your teammates on Graphite Graphite

@vegarsti vegarsti requested review from msf and adammilnesmith June 28, 2024 07:39
@vegarsti vegarsti force-pushed the report-errors-to-dune branch from 59469d3 to 594d404 Compare June 28, 2024 07:59
@vegarsti vegarsti force-pushed the report-errors-to-dune branch from 594d404 to 8afe1e0 Compare June 28, 2024 11:06
@vegarsti
Copy link
Member Author

I'm merging this so that I can build and push to DockerHub

@vegarsti vegarsti merged commit 85403d1 into main Jun 28, 2024
1 check failed
@vegarsti vegarsti deleted the report-errors-to-dune branch June 28, 2024 11:14
@@ -214,7 +214,7 @@ func (c *client) Close() error {
}

func (c *client) PostProgressReport(ctx context.Context, progress models.BlockchainIndexProgress) error {
Copy link
Contributor

@msf msf Jun 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when we complete this, we need to reset to zero the errors that we have aggregated so far.
otherwise at some point we'll have an OOM due to accumulated errors in memory.

this must be treated as an optimistic behavior, there will be errors we never capture (we hard crash), so we shouldn't be pedantic, we should really (try to) report and reset the errors we have.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but we do!

// Reset errors after reporting
i.info.RPCErrors = []ErrorInfo{}
i.info.DuneErrors = []ErrorInfo{}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants