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 universal events archiver function #226

Merged
merged 12 commits into from
Apr 22, 2024
Merged

Conversation

raylrui
Copy link
Contributor

@raylrui raylrui commented Apr 16, 2024

Feature/ universal-event-archiver

Features

Build a function to archive all events that go over the OrcaBus event bus, and write it as JSON to the dedicated S3 bucket Formatting the S3 key with year/month/day partitioning.
S3 object will be like: s3://{bucket_name}/events/{year}/{month}/{day}/{event_type}_{totalSeconds.microsecond}.json

To dos

  • need to check if all events follow the event_schemas, as we need retrieve the event type (or request title, request type)
  • time zone use utc or local time?
  • output bucket?
  • optional: DLQ problems? (need sqs queue for dlq)

@raylrui raylrui self-assigned this Apr 16, 2024
@reisingerf
Copy link
Member

Ah, very nice!
Already looks very good!

@reisingerf
Copy link
Member

OK, looks great!

Final suggestion / discusstion:
I know it's not really "stateful", but it's very closely linked to the event bus and we would not want to miss any events just because of "random" redeployments of the stateless resources. Therefore I'd suggest to:

  • move it under the stateful stack
  • add it to the event bus construct instead of keeping it separate

@victorskl your thoughts?

@raylrui raylrui marked this pull request as ready for review April 18, 2024 03:15
@raylrui
Copy link
Contributor Author

raylrui commented Apr 18, 2024

if we need add custom archiver, we can turn on Optional flag addCustomEventArchiver in the event bus construct with params of vpc, SG, destination Bucket:

addCustomEventArchiver?: boolean;
vpcProps?: VpcLookupOptions;
lambdaSecurityGroupName?: string;
archiveBucket?: string;

@victorskl
Copy link
Member

victorskl commented Apr 19, 2024

@raylrui Is this PR ready for review? Or, would you intend to complete remainder TODO items?

Could you also pls help me do favour; after you finalised, can you pls tick/press the "Re-request review" circle button, next to the reviewer name, pls. That way, I will get notification and, I will attend to it, asap. Thank you.

@raylrui
Copy link
Contributor Author

raylrui commented Apr 19, 2024

yes, @victorskl apology or that. So far this PR is ready for review, I will click the button, but nothing will happen when merged, as we don't open that flag and no bucket params defined.
I will make another PR if we wanna open that archiver and have bucket ready.

@victorskl
Copy link
Member

Thanks Ray, no apology need. I'll review in a tick, next & feedback.

@reisingerf
Copy link
Member

@victorskl FYI: just had a quick catch-up with Ray. He will add the archive bucket to the construct and create his own SG.
He'll also extend the eventbusConstruct.test.ts to check whether the addCustomEventArchiver flag is working as expected.

Copy link
Member

@victorskl victorskl left a comment

Choose a reason for hiding this comment

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

Comment to attend. And, could you do:

  • rebase or merge on top of current main
  • turn addCustomEventArchiver flag on
  • follow up archival bucket and, tests

Then, pls re-request review for me. Tq

securityGroups: [lambdaSG],
architecture: Architecture.ARM_64,
timeout: Duration.seconds(28),
index: 'universal_event_achiver.py',
Copy link
Member

Choose a reason for hiding this comment

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

ditto fix filename:

index: 'universal_event_archiver.py',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it should be a noun, like "archiver" sth, but happy to have any suggestion

Copy link
Member

Choose a reason for hiding this comment

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

Current filename is fine. It is just the typo.

Copy link
Member

Choose a reason for hiding this comment

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

Need fix filename typo:

universal_event_archiver.py

Copy link
Member

Choose a reason for hiding this comment

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

Also. Would be good, if you could unit test this handler. See token service cognitor as an example, if any. You can leverage botocore stubber.

Copy link
Member

Choose a reason for hiding this comment

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

And sanitize_string. Generally, all public functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I will look it through first and make update in unit test for next commit

@raylrui raylrui force-pushed the feature/universal-event-archiver branch from 1dcd5f1 to aba6f35 Compare April 19, 2024 02:54
Copy link
Member

@reisingerf reisingerf left a comment

Choose a reason for hiding this comment

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

LGTM

@raylrui raylrui force-pushed the feature/universal-event-archiver branch from f89eab8 to 422f770 Compare April 21, 2024 23:17
@raylrui
Copy link
Contributor Author

raylrui commented Apr 22, 2024

Hi team, recent updates on the archiver:

  1. add unit test for archiver function with Stubber
  2. refactor folder and code structure, and add makefile to make it easier to maintain
  3. open the addCustomEventArchiver flag and define specific bucket event-archive-bucketand lambdaSG OrcaBusEventArchiveSecurityGroup in config
  4. extend the cdk unit testing with addCustomEventArchiver and without addCustomEventArchiver in eventbusConstruct.test.ts
  5. add NagSuppressions.addResourceSuppressionsByPath for SharedStack in deployment.test.ts for the permission between archive lambda and archive bucket, as the use of AWS managed policies and wildcard permission break some default AwsSolutions Rules Pack checks.

@victorskl
Copy link
Member

thkz for update, Ray. I will review in tick after lunch. ETA 3pm

Copy link
Member

@victorskl victorskl left a comment

Choose a reason for hiding this comment

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

Nicely done, Ray.! Go for it.

Copy link
Member

@reisingerf reisingerf left a comment

Choose a reason for hiding this comment

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

Let's roll!

@raylrui raylrui merged commit c9bf1ee into main Apr 22, 2024
2 checks passed
@raylrui raylrui deleted the feature/universal-event-archiver branch April 22, 2024 04:42
@raylrui
Copy link
Contributor Author

raylrui commented Apr 22, 2024

I will try to test it after deployment finish...

@raylrui raylrui restored the feature/universal-event-archiver branch April 22, 2024 04:43
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.

Create universal event archive for OrcaBus events
4 participants