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: kinesis delivery stream updates #237

Merged

Conversation

travis
Copy link
Member

@travis travis commented Sep 22, 2023

merging into #191 - mostly put this up so @vasco-santos can review before I push it into the PR he opened!

  • implement date based partitioning
  • expand value and out struct definitions to allow for more interesting queries

@seed-deploy seed-deploy bot temporarily deployed to pr237 September 22, 2023 15:04 Inactive
@seed-deploy
Copy link

seed-deploy bot commented Sep 22, 2023

View stack outputs

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

I'm not really sure what I'm looking at but don't want to stand in the way here...

stacks/firehose-stack.js Show resolved Hide resolved
stacks/firehose-stack.js Show resolved Hide resolved
@travis
Copy link
Member Author

travis commented Sep 22, 2023

aha yea, tbh this is merging into @vasco-santos's PR and I mostly put it up for him to peek at before I pushed on top of #191 but this feedback is super helpful - I need to add some comments for the more esoteric stuff!

@seed-deploy seed-deploy bot temporarily deployed to pr237 September 22, 2023 18:03 Inactive
based on the queries I've put together this week
@seed-deploy seed-deploy bot temporarily deployed to pr237 September 22, 2023 21:48 Inactive
this seems to behave better with deleting stacks
@seed-deploy seed-deploy bot temporarily deployed to pr237 September 22, 2023 23:33 Inactive
Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

So glad to see this progress! Thanks a lot, everything looks great to me ❤️

Quite big digging on their docs to be able to map queries directly to the SST stack, this will make everything so much easier

@@ -145,8 +147,11 @@ export function UcanFirehoseStack({ stack, app }) {
},
],
},
// See https://docs.aws.amazon.com/athena/latest/ug/partition-projection-kinesis-firehose-example.html
prefix: 'logs/issuer=!{partitionKeyFromQuery:issuer}/',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think my logic here was to enable performant queries per issuer.

However, after reading through requirements docs, I think we should partition as you suggest instead. We can still query by issuer, but within a range, instead of all time. Which is good, given we already save space metrics anyway :)

parameters: {
classification: "json",
typeOfData: "file",
// See See https://docs.aws.amazon.com/athena/latest/ug/partition-projection-kinesis-firehose-example.html for more information on projection
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// See See https://docs.aws.amazon.com/athena/latest/ug/partition-projection-kinesis-firehose-example.html for more information on projection
// @see https://docs.aws.amazon.com/athena/latest/ug/partition-projection-kinesis-firehose-example.html for more information on projection

@travis travis force-pushed the feat/kinesis-delivery-stream-updates branch from 37a476a to 6b8504a Compare September 26, 2023 01:17
@travis travis merged commit 66ad70c into feat/kinesis-delivery-stream Sep 26, 2023
@travis travis deleted the feat/kinesis-delivery-stream-updates branch September 26, 2023 01:18
@seed-deploy seed-deploy bot temporarily deployed to pr237 September 26, 2023 01:23 Inactive
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.

3 participants