search

grafana / k6-jslib-aws

Javascript Library allowing to interact with AWS resources from k6 scripts
Apache License 2.0
18 stars 28 forks source link

Allow binary responses in `getObject` #107

Open KOConchobhair opened 1 month ago

KOConchobhair commented 1 month ago

Fixes #105

oleiade commented 1 month ago

Enforcing @joanlopez and me as reviewers as we've been following the source issue 👀

oleiade commented 1 month ago

See design considerations in #105.

oleiade commented 4 weeks ago

Hey @KOConchobhair 👋🏻

We have discussed it internally, and the maintainers agree we'd prefer the approach depicted in https://github.com/grafana/k6-jslib-aws/issues/105#issuecomment-2173486709.

If you have the capacity, we'd greatly appreciate it if you could align this PR's design. Otherwise, that's also fine, and we'd likely proceed with it ourselves. Just let us know 🙇🏻

KOConchobhair commented 4 weeks ago

Sorry for the delay, I was a little confused by your suggestion in https://github.com/grafana/k6-jslib-aws/issues/105#issuecomment-2173486709 since Accept is not even one of the headers listed at the S3 link. But i guess you are saying that it gives you the path to support them in the future.

I will just state that this is not an S3 API problem at all, its a very specific problem related to the inner workings of k6. So specifying an Accept header that doesn't actually change how S3 responds and only impacts the parsing on the k6 side is a little odd (but again, its putting in place the API for future stuff which is nice to not break the API again). I think this makes it a little difficult to support all types of binary headers (so we'll have to add images too, so i guess image/*)

but this is a blocker for me using k6 how I want to in my project so I'll implement since I'd really like to continue using k6 😄

oleiade commented 4 weeks ago

We know this is not an official AWS header, but it is an official HTTP header, and it indicates to the server the response type you expect to receive back, so it is a good fit.

This is also why we call it additionalHeaders and not requestParameterHeaders: It allows you to add any headers to the request, not only AWS ones. Not being able to set the underlying HTTP request's headers has caused a couple of issues in this project over time. It is actually a design we could probably extend to the AWSClient itself to a certain extent (although I can't necessarily think on top of my head of another method that would have the same need to express its result, which should be treated as bytes).

This way of doing it feels like a better mitigation than aligning with the responseType approach of the k6 HTTP client, which is not ideal in my experience and, as we're redesigning our HTTP client, might go away in the future.

Regarding the different MIME types supported, my assumption was that using the Accept header in this case was meant more as a convention than a strict requirement we would expect S3 to comply with: " If you set the Accept header, the S3 client will return binary content, otherwise text."

I'll look at the official support AWS headers to see if another candidate I might have overlooked shows up 🙇🏻

KOConchobhair commented 4 weeks ago

Regarding the different MIME types supported, my assumption was that using the Accept header in this case was meant more as a convention than a strict requirement we would expect S3 to comply with: " If you set the Accept header, the S3 client will return binary content, otherwise text."

Just thinking from a k6 user perspective though, giving something as broad as an Accept header in this API which allows me to set it to { 'Accept': 'application/json' } and then that would mean k6 will interpret as binary, that would be surprising and non-intuitive behavior if I were writing a k6 script.

oleiade commented 4 weeks ago

For clarification, as mentioned in my initial comment only "Accept: application/octet-stream" would lead to the content being treated as binary.

KOConchobhair commented 4 weeks ago

oh duh, okay so rephrased "If you set the Accept header (to application/octect-stream), the S3 client will return binary content, otherwise text."

oleiade commented 3 weeks ago

Hey @KOConchobhair 👋🏻

Will you have capacity to update the PR according to our discussions? 🙇🏻

PS: I will be away for the next two weeks, but @joanlopez will be able to follow-up on this in the meantime.

KOConchobhair commented 1 week ago

this is still on my list but fell down the priority a bit, hopefully i can get back to it this week