Added support for Kinesis #33

autometa101 · 2023-01-24T16:11:07Z

Adding API Support for Kinesis.
It will support all APIs from https://docs.aws.amazon.com/kinesis/latest/APIReference/API_Operations.html

CLAassistant · 2023-01-24T16:11:14Z

All committers have signed the CLA.

oleiade

Great work 🎉 👏🏻 Thanks a lot for your contribution, much appreciated 🙇🏻

I've left some comments regarding how the implementation has been approached. Nothing major, but I would still like to address it before we merge it. Let me know if you have any questions, if I can support you in any ways 👍🏻

src/internal/kinesis.ts

tests/internal/kinesis.js

README.md

oleiade

Great work with the last batch of changes. Thanks a lot for putting the work into it 🙇🏻

Here's another batch of comments and ideas I'd like to see addressed. That could come across as a lot, but as maintainers, we'll have to own, document, and maintain this code long-term, and I want to ensure we get everything right. I hope you understand 🙏🏻

In general, most of my comments are either questions, cosmetic change requests, or questions. Let me know if you have any questions or feedback 👀

A more general comment:

I haven't decided on it yet, but I find the whole *Request, *Response interfaces a bit much to process, document, and maintain.

Furthermore, it's pretty low-level, and relatively close to the actual underlying request/responses format, whereas, at k6, we deeply care about delivering the most friendly-user interfaces possible. In the context of the jslib-aws, we prefer to provide fewer but higher-level abstractions.

I will provide comments directly on the code by the beginning of next week. But my shallow and overarching comment, as of right now, is that I would prefer to have some types/classes that align more with the data types described by the AWS Kinesis Data Types Documentation, and less with the underlying bodies of the requests and response. I would like to see them returned directly by the methods exposed to the user rather than the current *Response type. That means that instead of a ListShardsResponse, for instance, I'd prefer a method that returns an Array<Shard>.

I have weaker opinions on this, but the same goes for the *Request types. I see their value and convenience to us, the developers. Still, they also obfuscate the actual API and don't align with our goal for the project to make understanding and using the APIs straightforward. In many cases, those interfaces also contain just a couple of members. They could easily be replaced by single arguments (which are also much easier to document when I update the document website).

I hope these comments make sense. Let me know if you need any clarification or support with it, I would be happy to help 🙇🏻

oleiade · 2023-02-03T16:42:07Z

README.md

@@ -14,6 +14,8 @@ At the moment, this library provides the following:
 * `KMS`: allows to list KMS keys and generate a unique symmetric data key for use outside of AWS KMS
 * `SSM`: allows to retrieve a parameter from AWS Systems Manager
 * `V4 signature`: allows to sign requests to amazon AWS services
+* `KinesisClient`: allows all APIs for Kinesis available by AWS.


Suggested change

* `KinesisClient`: allows all APIs for Kinesis available by AWS.

* `KinesisClient`: use it to interact with the AWS Kinesis service.

oleiade · 2023-02-03T16:43:27Z

src/internal/kinesis.ts

+            'Content-Type': 'application/x-amz-json-1.1',
+        }
+
+        return new Proxy(this, {


My understanding is that we don't need this function anymore. Can we please drop it? 🙏🏻 🙇🏻

This is growing lib, and would not like to limit the functions until all are implemented, specially when its already available easily. I hope this would be fine, what do you suggest ?

I appreciate where you come from @autometa101. However, as mentioned in a previous iteration, we would like to avoid having "magic" code (as we need to document the available API to users in k6's documentation), and we would also like to avoid code meant for a future use case.

Let's, please, drop that call for now 🙇🏻

oleiade · 2023-02-03T16:49:54Z

src/internal/kinesis.ts

+     * @param {CreateStreamRequest} request - CreateStreamRequest - The request object that will be
+     * sent to the server.
+     */
+    createStream(request: CreateStreamRequest): void {


I'd prefer to see this method and the rest of the following high-level and public methods appear right after the constructor 👍🏻

oleiade · 2023-02-03T16:54:03Z

src/internal/kinesis.ts

+
+
+    /**
+     * It makes a request to the AWS API.


Suggested change

* It makes a request to the AWS API.

* Sends a signed request to AWS Kinesis API.

oleiade · 2023-02-03T16:56:27Z

src/internal/kinesis.ts

+     * @param {JSONObject} options - JSONObject = {}
+     * @returns A RefinedResponse<ResponseType | undefined>
+     */
+    RequestOperation(target: string,


Suggested change

RequestOperation(target: string,

_request(action: KinesisAction,

As hinted in ☝🏻 I would like a dedicated type describing and delimiting the supported Kinesis actions: type KinesisAction = 'CreateStream' | 'etc...'.

Like we did in https://github.com/grafana/k6-jslib-aws/blob/main/src/internal/s3.ts#L376. We named them "operation" in the S3Client too, but we regret that terminology as it is not aligned with AWS, which refers to them as "actions".

oleiade · 2023-02-03T17:04:11Z

src/internal/kinesis.ts

+    }
+
+    /**
+     * It returns a list of streams.


Suggested change

* It returns a list of streams.

* Lists your Kinesis data streams.

*

oleiade · 2023-02-03T17:05:31Z

src/internal/kinesis.ts

+    }
+
+    /**
+     * It takes a request object and returns a response object.


Suggested change

* It takes a request object and returns a response object.

* Writes multiple data records into a Kinesis data stream in a single call.

*

* Use this operation to send data into the stream for data ingestion and processing.

*

oleiade · 2023-02-03T17:06:03Z

src/internal/kinesis.ts

+    }
+
+    /**
+     * It gets the records from the database.


Suggested change

* It gets the records from the database.

* Gets data records from a Kinesis data stream's shard.

*

oleiade · 2023-02-03T17:06:31Z

src/internal/kinesis.ts

+    }
+
+    /**
+     * It lists the shards in a stream.


Suggested change

* It lists the shards in a stream.

* Lists the shards in a stream and provides information about each shard.

*

oleiade · 2023-02-03T17:07:32Z

src/internal/kinesis.ts

+
+
+    /**
+     * 


Suggested change

*

* Gets an Amazon Kinesis shard iterator. A shard iterator expires 5 minutes after it is returned to the requester.

*

I'm not familiar with Kinesis: this returns an "iterator", which seems to be some id by the looks of it. Do we have any methods of making any use of it, though? Otherwise, we'd drop this method altogether. Once again I'm not familiar with Kinesis, so, looking forward to your 💡 here 🙏🏻

oleiade · 2023-02-28T13:17:48Z

Hey @autometa101, thanks for the further work you've put into this 🤝 and sorry for coming back to this so late, we had a company event in the meantime. We truly value your contribution and have been thinking about how to approach merging it in the main branch in the best conditions. The conclusion was that we would like to merge this PR soonish and incorporate the changes and documentation we think are necessary ourselves. The feature will therefore land in the next upcoming version of the library which has no release date set just yet 👍🏻

Let me know if you have any opposition to this plan, questions, or feedback. Otherwise, we'll just proceed. Cheers! 🚀

autometa101 · 2023-02-28T16:55:51Z

Hi @oleiade,

I was extensively using this library (branch) to publish few million messages in last days,
where I found 2 issues as following.

Too big request error, where k6/http post is throwing up the request error.
AWS is throwing error EOF, for the similar reason.

However, I was not able to continue working on the requested changes to remove all methods and allow only limited with details. If there is no changes in direction, I am planning to continue development in next week.
[p.s As User, I found very useful to just add the direct call to API if method is not available.]

oleiade · 2023-03-07T15:22:32Z

Hey @autometa101

Thanks for the heads-up 🙇🏻

Can you open GitHub issues about those specific issues you've encountered? If anything, they will make them visible to users using the Kinesis client once it's merged. We already have users asking about this feature, which will help us all keep track of limitations and potential issues users would run into.

My rationale was that the client is already in a usable state, and most of the blocking points are about how the code is structured, etcetera. We figured we would go faster doing those ourselves.

Here's how we're going to proceed then:

I'll merge this in main
I'll expect you to open the mentioned issues
I'll make sure to publish the changes I make to the client in the form of a PR (I'll ask for your input to make sure that it works, etcetera).
merge it and publish a 0.8.0 version of the library

oleiade self-requested a review January 24, 2023 16:22

oleiade self-assigned this Jan 24, 2023

oleiade added the enhancement New feature or request label Jan 24, 2023

autometa101 force-pushed the main branch from 7c6e742 to 16e2e6b Compare January 24, 2023 17:49

autometa101 added 2 commits January 24, 2023 23:30

adding support for kinesis

5f44d4a

rerun webpack to resolve merge conflict

30da860

autometa101 force-pushed the main branch from 16e2e6b to 30da860 Compare January 24, 2023 18:03

oleiade requested changes Jan 25, 2023

View reviewed changes

update for repo standardization

90f11bb

autometa101 requested a review from oleiade January 29, 2023 20:10

oleiade requested changes Feb 3, 2023

View reviewed changes

autometa101 added 2 commits February 17, 2023 11:50

logging error for k5

075fed3

logging error for k6

56d0d2a

autometa101 marked this pull request as draft February 28, 2023 16:40

oleiade added this to the 0.8.0 milestone May 2, 2023

oleiade mentioned this pull request May 5, 2023

Add support for Kinesis #46

Merged

oleiade closed this in #46 May 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for Kinesis #33

Added support for Kinesis #33

autometa101 commented Jan 24, 2023

CLAassistant commented Jan 24, 2023 •

edited

Loading

oleiade left a comment

oleiade left a comment

oleiade Feb 3, 2023

oleiade Feb 3, 2023

autometa101 Feb 6, 2023 •

edited

Loading

oleiade Feb 6, 2023 •

edited

Loading

oleiade Feb 3, 2023

oleiade Feb 3, 2023

oleiade Feb 3, 2023

oleiade Feb 3, 2023

oleiade Feb 3, 2023

oleiade Feb 3, 2023

oleiade Feb 3, 2023

oleiade Feb 3, 2023

oleiade Feb 3, 2023

oleiade Feb 3, 2023

oleiade commented Feb 28, 2023

autometa101 commented Feb 28, 2023 •

edited

Loading

oleiade commented Mar 7, 2023 •

edited

Loading

	* `KinesisClient`: allows all APIs for Kinesis available by AWS.
	* `KinesisClient`: use it to interact with the AWS Kinesis service.

	* It makes a request to the AWS API.
	* Sends a signed request to AWS Kinesis API.

	RequestOperation(target: string,
	_request(action: KinesisAction,

	* It returns a list of streams.
	* Lists your Kinesis data streams.
	*

-     * It takes a request object and returns a response object.
+     * Writes multiple data records into a Kinesis data stream in a single call.
+     *
+     * Use this operation to send data into the stream for data ingestion and processing.
+     *

	* It gets the records from the database.
	* Gets data records from a Kinesis data stream's shard.
	*

	* It lists the shards in a stream.
	* Lists the shards in a stream and provides information about each shard.
	*

	*
	* Gets an Amazon Kinesis shard iterator. A shard iterator expires 5 minutes after it is returned to the requester.
	*

Added support for Kinesis #33

Added support for Kinesis #33

Conversation

autometa101 commented Jan 24, 2023

CLAassistant commented Jan 24, 2023 • edited Loading

oleiade left a comment

Choose a reason for hiding this comment

oleiade left a comment

Choose a reason for hiding this comment

A more general comment:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autometa101 Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

oleiade Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleiade commented Feb 28, 2023

autometa101 commented Feb 28, 2023 • edited Loading

oleiade commented Mar 7, 2023 • edited Loading

CLAassistant commented Jan 24, 2023 •

edited

Loading

autometa101 Feb 6, 2023 •

edited

Loading

oleiade Feb 6, 2023 •

edited

Loading

autometa101 commented Feb 28, 2023 •

edited

Loading

oleiade commented Mar 7, 2023 •

edited

Loading