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

feat: make the timeout for event observers configurable #5286

Merged
merged 3 commits into from
Oct 10, 2024

Conversation

obycode
Copy link
Contributor

@obycode obycode commented Oct 8, 2024

Description

Adds an optional timeout_ms to the configuration for event observers, allowing users to set a unique timeout for each observer.

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@obycode obycode requested review from a team as code owners October 8, 2024 16:59
@CharlieC3
Copy link
Member

@obycode Would it be better to err on the side of flexibility and change the default to something larger that wouldn't likely cause issues syncing a large block to a low-powered observer, like an API with minimal resources?
Instead of setting the default to 1 second, it might be better to set it to 60 seconds by default and let others rein it in if needed. Thoughts on this?

jcnelson
jcnelson previously approved these changes Oct 8, 2024
@obycode
Copy link
Contributor Author

obycode commented Oct 8, 2024

@obycode Would it be better to err on the side of flexibility and change the default to something larger that wouldn't likely cause issues syncing a large block to a low-powered observer, like an API with minimal resources? Instead of setting the default to 1 second, it might be better to set it to 60 seconds by default and let others rein it in if needed. Thoughts on this?

Yeah, that could make sense. I chose 1s as the default since that what it was hard-coded to before this change, so this change would have no effect.

The other problem with defaulting to 60s is that that could be surprising, since it halts progression of a node, a 60s timeout before seeing any logs is a lot, given that blocks will be coming in on the order of seconds.

@obycode obycode requested a review from a team October 8, 2024 17:26
@wileyj
Copy link
Contributor

wileyj commented Oct 8, 2024

for what it's worth, confirmed that this change does allow genesis sync with the API (moved past block 0)

@CharlieC3
Copy link
Member

The other problem with defaulting to 60s is that that could be surprising, since it halts progression of a node, a 60s timeout before seeing any logs is a lot, given that blocks will be coming in on the order of seconds.

I agree, but I think that says more about the logging from when a timeout is reached rather than the default setting itself. If the log when an observer timeout is specific, like "timeout reached (60s) trying to send block 12345 to observer my.domain.com:3999", it wouldn't be very confusing to debug.

Not suggesting this is needed now, but just thinking it would pose less consternation to set a wide boundary now and come back to it later with improved logging, rather than set a narrow boundary now and cause many operators to update all of their configs since 1s is likely too low for many people, and JW has already seen payloads take close to 30 seconds.

@obycode obycode added this pull request to the merge queue Oct 10, 2024
Merged via the queue into develop with commit ae3d6d5 Oct 10, 2024
1 check passed
@pc-quiknode
Copy link

@obycode timeout_ms doesn't work with 3.0.0.0.0 for some reason and I keep getting this error with Nakamoto Testnet nodes

WARN [1729752052.503256] [testnet/stacks-node/src/event_dispatcher.rs:496] [main] Event dispatcher: connection or request failed to localhost:3700 - Custom { kind: WouldBlock, error: "Timed out while receiving request" }, backoff: 3s, attempts: 18

@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants