-
Notifications
You must be signed in to change notification settings - Fork 290
Adding send action for kinesis destination #3397
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3397 +/- ##
==========================================
+ Coverage 80.33% 80.36% +0.02%
==========================================
Files 1267 1485 +218
Lines 25246 28785 +3539
Branches 5185 6066 +881
==========================================
+ Hits 20282 23133 +2851
- Misses 4176 4844 +668
- Partials 788 808 +20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| credentials: credentials | ||
| }) | ||
|
|
||
| await client.send(command) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the AbortSignal Support similar to s3 destination?
#3381
This will help us standardize timeouts in the system as part of Flexible Request Durations
: https://docs.google.com/document/d/1snhMygiEaMJ2QwC68PjNTmNQdvUhwhqcbTVzlJY4XQA/edit?tab=t.xja93wmhydze
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo, kinesis destination is very different from S3 or SFTP destinations because in S3/SFTP destinations, we upload very large objects, where the latency will be high and there is a chance that the connection might hang. In Kinesis API call, the max records can't exceed beyond 10 MB / 500 records per call, thus the corresponding latency will be very low (in order of few seconds). I plan to add timeout by connectionTimeout and socketTimeout which is simple to implement that AbortSignal. JIRA https://twilio-engineering.atlassian.net/browse/STRATCONN-6335
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Irrespective of payload sizes, the abortsignal should be implemented as it also handles centrifuge side timeouts/request cancellations, making sure all process are reliably cancelled. However, you can address this in a separate PR as a ticket for there seems to be a separate ticket for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, let's take it separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acking that abort signal will be implemented later.
joe-ayoub-segment
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Mohammed - I reviewed some of the code and left some comments. But decided to ping you to catch up in person...
packages/destination-actions/src/destinations/aws-kinesis/send/index.ts
Outdated
Show resolved
Hide resolved
packages/destination-actions/src/destinations/aws-kinesis/send/index.ts
Outdated
Show resolved
Hide resolved
packages/destination-actions/src/destinations/aws-kinesis/utils.ts
Outdated
Show resolved
Hide resolved
packages/destination-actions/src/destinations/aws-kinesis/utils.ts
Outdated
Show resolved
Hide resolved
packages/destination-actions/src/destinations/aws-kinesis/utils.ts
Outdated
Show resolved
Hide resolved
| const validatePartitionKey = (partitionKey: string): void => { | ||
| if (!partitionKey) { | ||
| throw new IntegrationError( | ||
| `Partition Key is required in the payload to send data to Kinesis.`, | ||
| 'PARTITION_KEY_MISSING', | ||
| 400 | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function shouldn't be required. partition_key is a required field, so only payloads containing a value will be passed into the perform() and performBatch() functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo, it's better to write necessary validations than to only rely on clients. It will be helpful during local testing or traffic from other source than centrifuge (in future)
| region: awsRegion, | ||
| credentials: credentials, | ||
| requestHandler: new NodeHttpHandler({ | ||
| requestTimeout: KINESIS_COMMAND_TIMEOUT_MS // timeout in milliseconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the correct timeout value to set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will revisit it in the ticket https://twilio-engineering.atlassian.net/browse/STRATCONN-6335
| credentials: credentials | ||
| }) | ||
|
|
||
| await client.send(command) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acking that abort signal will be implemented later.
|
PR deployed |
|
Hi @joe-ayoub-segment it appears that this change has broken our entire segment setup as process.env is trying to be read a browser environment |
|
@joe-ayoub-segment this seems to have broken our segment setup. The erroring file is in Destination/actions-shared/dist/aws-config/index.js, which is Segment's internal "Actions / Destinations" bundle. |
This reverts commit dc96058.
|
Hello @viralpickaxe @johndellavecchia , Thank you for reporting this issue. We sincerely apologize for the disruption this change has caused to your Segment setup. We are currently prioritizing a revert of the recent change to resolve the process.env issue occurring in the browser environment. We are working on deploying the fix as quickly as possible. We will provide an update here once the revert is complete and the fix is live. Thank you for your patience and understanding. |
Testing