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.

Sign up for GitHub

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

Jump to bottom

Bridge: add /events poller #1363

Merged
merged 6 commits into from
Jul 11, 2024
Merged

Bridge: add /events poller #1363

merged 6 commits into from
Jul 11, 2024

Conversation

svix-onelson
Copy link
Contributor

@svix-onelson svix-onelson commented Jul 9, 2024 •
edited
Loading

Helps with https://github.com/svix/monorepo-private/issues/8654

Depends on client lib updates (not yet available).

I'd call this "quick and dirty" but it was actually a bit of a hefty lift to pull together. It is however fairly dirty.

The want is to be able to pull messages from /events and feed them into a receiver output.
The flow we'd see with SenderInput paired with ReceiverOutput is what we want, but the existing typing didn't allow for this.

Refactors were needed to:

  • lift out the parts from the HTTP server that run payloads through transformations then forward to an output.
  • update config to account for a new type of receiver: a "poller".
  • new traits/types for the poller to differentiate it from the standard webhook receiver (to ensure we don't accidentally pass one through and try to bind it to a URL).
  • Lots of "connective tissue" in the form of converters between config values and concrete ones that can actually do things.

Some of the "connective tissue" exists purely to mimic bits and pieces that existed for either the other receivers or senders (remember, this case is odd in that it's similar to both).

Refactorings aside, the poller itself boasts an exponential backoff for both error cases (either from /events or from the output) as well as for the case where the /events iterator is "done."

This diff comes with a promise that we will (soon) give these additions another look at clean up the stuff that doesn't make sense or feels redundant.

@svix-onelson svix-onelson force-pushed the onelson/bridge-events branch 4 times, most recently from 681bbf1 to ba93978 Compare July 9, 2024 22:31
@svix-onelson svix-onelson changed the title WIP: /events poller Bridge: add /events poller Jul 10, 2024
@svix-onelson svix-onelson marked this pull request as ready for review July 10, 2024 05:09
@svix-onelson svix-onelson requested a review from a team as a code owner July 10, 2024 05:09
svix-jplatte
svix-jplatte previously approved these changes Jul 10, 2024
View reviewed changes
Copy link
Member

@svix-jplatte svix-jplatte left a comment

Choose a reason for hiding this comment

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

Nothing looks wrong, left some cleanup notes though (can be addressed in another PR).

bridge/svix-bridge/src/config/mod.rs Outdated Show resolved Hide resolved
bridge/svix-bridge/src/main.rs Outdated Show resolved Hide resolved
bridge/svix-bridge/src/webhook_receiver/mod.rs Outdated Show resolved Hide resolved
bridge/svix-bridge/src/webhook_receiver/mod.rs Outdated Show resolved Hide resolved
bridge/svix-bridge/src/webhook_receiver/mod.rs Outdated Show resolved Hide resolved
svix-onelson
svix-onelson commented Jul 10, 2024
View reviewed changes
Copy link
Contributor Author

@svix-onelson svix-onelson left a comment

Choose a reason for hiding this comment

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

I'll take a look at making the itertools change while waiting on the rest of this stuff to coalesce. 🙏🏻

bridge/svix-bridge/src/main.rs Show resolved Hide resolved
bridge/svix-bridge/src/config/mod.rs Outdated Show resolved Hide resolved
bridge/svix-bridge/src/main.rs Outdated Show resolved Hide resolved
@svix-onelson
Copy link
Contributor Author

OSS Review: itertools

This popular extension crate adds new methods for iterators.
Useful in this case as it cleans up a previous call to fold() designed
to partition a collection into 2 collections that can't be expressed
cleanly with a plain partition().

The selected version is not the newest, but it aligns with a selected
version already included in the project transitively.

cc @svix-james @jaymell

@svix-onelson svix-onelson force-pushed the onelson/bridge-events branch 4 times, most recently from c80d904 to 4bca290 Compare July 10, 2024 23:01
@svix-onelson svix-onelson mentioned this pull request Jul 10, 2024
Merged
@svix-onelson svix-onelson changed the base branch from main to onelson/spec-bump July 10, 2024 23:05
Base automatically changed from onelson/spec-bump to main July 10, 2024 23:14
svix-onelson and others added 4 commits July 11, 2024 10:02
@svix-onelson
Bridge: add /events poller
48bbd21 
I'd call this "quick and dirty" but it was actually a bit of a hefty
lift to pull together. It is however fairly dirty.

The want is to be able to pull messages from `/events` and feed them
into a receiver output.
The flow we'd see with `SenderInput` paired with `ReceiverOutput` is
what we want, but the existing typing didn't allow for this.

Refactors were needed to:
- lift out the parts from the HTTP server that run payloads through
  transformations then forward to an output.
- update config to account for a new type of receiver: a "poller".
- new traits/types for the poller to differentiate it from the standard
  webhook receiver (to ensure we don't accidentally pass one through
  and try to bind it to a URL).
- Lots of "connective tissue" in the form of converters between config
  values and concrete ones that can actually do things.

Some of the "connective tissue" exists purely to mimic bits and pieces
that existed for either the other receivers or senders (remember, this
case is odd in that it's similar to both).

Refactorings aside, the poller itself boasts an exponential backoff for
both error cases (either from `/events` or from the output) as well as
for the case where the `/events` iterator is "done."

This diff comes with a promise that we will (soon) give these additions
another look at clean up the stuff that doesn't make sense or feels
redundant.
@svix-onelson @svix-jplatte
code review: update tracing attr spellings
5ccb055 
Co-authored-by: Jonas Platte <[email protected]>
@svix-onelson
Bridge: add itertools for partition_map
941f69a 
OSS Review: itertools

This popular extension crate adds new methods for iterators.
Useful in this case as it cleans up a previous call to `fold()` designed
to partition a collection into 2 collections that can't be expressed
cleanly with a plain `partition()`.

The selected version is not the newest, but it aligns with a selected
version already included in the project transitively.
@svix-onelson
Bridge: decode svix events poller's "bridge config" from base64
47e35a7 
There's a new authtoken endpoint for creating fresh events
subscriptions. In the response is a base64 encoded json structure
including the authtoken to use, the subscription id, and the app id -
everything we need to build an appropriate URL for fetching events from
the stream.
@svix-onelson
Bridge: bump rust sdk to 1.25
6bfccbe 
This gives access to the needed beta `events_subscription` API.
@svix-onelson
Bridge: factor backoffs to make sure we set sleep_time consistently
5536e9a 
There were some gaps in the initial implementation. This switches to
setting a flag within the message handling loop and breaking the loop so
that the sleep time penalty (or reset) can be assessed at the bottom of
the loop more consistently.

Mostly this is about making sure any single message that has a problem
makes the whole iterator batch retry.
@svix-onelson
Copy link
Contributor Author

Some relatively new stuff since last review added, starting with 47e35a7

  • new config using the new "subscription token" concept
  • revisions to the backoff/sleep handling in the loop

@svix-onelson svix-onelson merged commit 269153d into main Jul 11, 2024
6 checks passed
@svix-onelson svix-onelson deleted the onelson/bridge-events branch July 11, 2024 18:45
@svix-onelson svix-onelson mentioned this pull request Jul 11, 2024
Merged
svix-onelson added a commit that referenced this pull request Jul 12, 2024
@svix-onelson
Bridge: use batch count=0 to signal backoff, not "done"
0916feb 
Looks like the `done` field doesn't signify we're at the end of the
topic like I originally thought.

Using the count as a proxy for this works well enough for now, but may
not in the future if we expose filters like channels/event types.

Follow-on from #1363
svix-onelson added a commit that referenced this pull request Jul 12, 2024
@svix-onelson
Bridge: use batch count=0 to signal backoff, not "done"
1cf6dc4 
Looks like the `done` field doesn't signify we're at the end of the
topic like I originally thought.

Using the count as a proxy for this works well enough for now, but may
not in the future if we expose filters like channels/event types.

Follow-on from #1363
@svix-onelson svix-onelson mentioned this pull request Jul 12, 2024
Closed
svix-onelson added a commit that referenced this pull request Jul 12, 2024
@svix-onelson
Bridge: log svix client errors as error, not trace (#1368)
a99a542 
We've got 3 places where we log errors in the poller loop and this one
somehow wound up logging as `trace!` instead of `error!` like the rest.

Follow-on from #1363
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svix-james svix-james svix-james approved these changes

@svix-jplatte svix-jplatte Awaiting requested review from svix-jplatte

Assignees
No one assigned
Labels
open source review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@svix-onelson @svix-james @svix-jplatte