Betterment / journaled

A Rails engine to reliably deliver loosely-ordered schematized events to Amazon Kinesis via Delayed::Job
MIT License
13 stars 9 forks source link

Replace `app_name` config with direct `stream_name` configurability #21

Closed smudge closed 2 years ago

smudge commented 2 years ago

Summary

This replaces default_app_name, journaled_app_name, and other app_name usage with default_stream_name, journaled_stream_name, and stream_name (respectively). It also moves off of an ENV-vars-by-default approach for stream configuration, to more of a BYO-ENV-vars strategy (where you can still reference the old ENV vars if you want).

Naturally, this prompts a major version bump to 4.0, and upgrade instructions have been added the README.

Why?

We've started to realize that treating "app names" and kinesis streams as 1:1 pairings is a somewhat limiting convention. We already have at least one use case now where we actively want to send a subset of an app's events to a different stream (with different firehose routing rules), and as it stands we'd need to essentially invent a new "app name" and also set a new environment variable in order to make that happen. So this new config removes a layer of indirection there.

If you take a look at the upgrade instructions in the README, you'll see that the previous app-name-based world is still fully supported -- the old environment variables can be dropped directly into the new configurations, just with fewer baked-in assumptions about ENV conventions by the gem itself.

(There are more steps we could take to allow for region and IAM role configurability across event types, but for now we'll assume that these streams are still colocated in the same region and accessible with the same role.)

Other Information

/domain @Betterment/journaled-owners /platform @jmileham @coreyja @danf1024 @effron /cc @matsuimp

nanda-prbot commented 2 years ago

Needs somebody from @Betterment/journaled-owners to claim domain review Needs somebody from @jmileham, @danf1024, and @effron to claim platform review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

HOW TO: Claim a Review

(Ignored request for platform review from coreyja. See here for our list of platform experts.)

effron commented 2 years ago

<<platformLGTM!

nanda-prbot commented 2 years ago

Needs somebody from @Betterment/journaled-owners to claim domain review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

HOW TO: Claim a Review

(Ignored request for platform review from coreyja. See here for our list of platform experts.)

samandmoore commented 2 years ago

<<domain

nanda-prbot commented 2 years ago

Needs @samandmoore to provide domain review

When you finish a round of review, be sure to say you've finished or sign off on the PR, e.g.:

TAFN or DomainLGTM

If you're too busy to review, unclaim the PR, e.g.:

@myname >> domain

HOW TO: Give Feedback

samandmoore commented 2 years ago
  • Worth noting, I made the choice to bake stream_name into the Journaled::DeliveryJob handlers. This means that jobs will use the stream names that the events were configured to use at the time they were enqueued. My thought is that this is the more desirable trade-off (I find it easier to reason about at least), but it does mean that if you deploy a change to default_stream_name or journaled_stream_name, it won't kick in for jobs that were already enqueued prior to the deploy.

I also find this intuitive.

  • Because this is a major version bump, I removed the old Journaled::Delivery "performable" class, since it has been succeeded by the Journaled::DeliveryJob ActiveJob. This means that anyone upgrading directly from 2.5.0 or below to 4.0 (this new version) will see errors if they have any jobs actively queued. I've added a recommendation to the README to upgrade one major version at a time, so 2.x -> 3.x -> 4.x would be the safest path.

šŸ‘šŸ»