Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Design document for the new HTTP API #2971
Design document for the new HTTP API #2971
Changes from 1 commit
0560b4f
529e4fd
1084875
d2e5e2c
7c49aef
0d8233a
98841cb
d5ffcd2
336abcd
1e71d7e
5b7e313
8c97fe0
5582425
ba3383e
81034aa
0fde824
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If they are confirmed, do you plan to include the concrete proposal for them in this document?
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.
#936 would be fixed by the
version
property.h2c
could be supported via another property, so maybe:Though this would mean that you couldn't pass an already connected
socket
, or it would force a reconnection. 🤔 Not sure, we need to decide how to handle the whole Socket/Transport API, and if we're implementing that first, or exposing it later. Once we have a better idea of what we're doing there, making h2c configurable would be a relatively minor addition.But to answer your question: yes, we should have proposals for issues we plan to work on in phase 1 before merging this PR.
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.
From what I know, HTTP version negotiation happens at a lower level, so I am not sure if the
Client
is the place where we should have aversion
property 🤔 On the network or transport level is probably more appropriate 🤔 Though I am not sure if we should even specify these things in this design doc, a PoC seems like a better place to figure them outThere 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.
Right, it can happen during TLS negotiation, as part of ALPN. But HTTP/1.1 connections can also be upgraded via a header. I'm conflicted about it as well, though it definitely should be somewhere lower level as well.
Maybe part of
TLS.connect()
?This way you could force HTTP/1.1 over TLS, even if the server supports HTTP/2. This could be simplified to a
versions
array, instead ofalpn
.Since
h2c
must be negotiated via anUpgrade
header, and can only be done without TLS, then something like theinsecure
flag above on theClient
itself would be the way the go, in which case we should also keep theversion
property. It wouldn't make sense to specify that as a, say,TCP.open()
option.But I agree that we don't need to agree on every single detail to start working on the PoC. We can iron out these details as we make progress.
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.
Just to note that the
UPGRADE
header is only used for (in practice) websockets andh2c
without "prior knowledge".The usuall http2 upgrade happens in practice in the tls handshake and h2c with prior knowledge just starts talking http2 directly.
http3 being over UDP means that the upgrade is basically a completely new connection. Except again if "prior knowlege" is used in which case it is still on a new connection, it just skips the first one ;).
I am not certain where this should be configured ... or even if it should be one place or if it should be solved with some more involved setup.
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.
I think we can promote them.
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.
@codebien See 81034aa.
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.
In 0fde824, I added TLS options to
TCP.open()
, instead of havingTLS
be a separatek6/x/net
class.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.
I guess this is the part with all the socket and dns stuff?
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.
It's not clear yet, so we should decide how to handle that.
The DNS lib? Sure, it can be done here, as it's not critical. For the socket lib, I'm not so sure, and think it should be done earlier, maybe even in phase 1.
This is partly a reply to your top comment, but I think we should structure the Go API in such a way that it mirrors the JS API, so that exposing things like the Sockets API would just be a matter of adding JS bindings to it. If we don't think about how the API will look from the JS side and drive the implementation based on that, then we might end up with a state that uses Go semantics and is difficult to later expose to JS.
This is why I think we should start with the lower level APIs the HTTP API depends on first, and then build on top of it. I added an introduction to the Design section that touches on why this is important.
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.
Ah, forgot to mention that phase 3 would essentially be for features we consider lower priority. The high priority features would be done in phases 1 and 2, but we should decide which features should be done when. The spreadsheet I shared attempts to do this, so we can start there.
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.
I kind of don't see the benefit of any of this especially as part of this proposal.
While some or all of those changes (won't call them improvements as I personally don't like them), might be added. None of those IMO are a good reason to not make the API generally available.
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.
Which part are you specifically referring to? All 3 points of the Goals section, or just the Fetch point?
I'm surprised you object to the Fetch API, since you opened #2424. 😉
I don't understand why this is such a hot topic...
First of all, this is not a blocker for making the API generally available. It will be available to anyone who wants to use the extension starting from phase 1. And we now agreed that the extension should be merged into k6 as an experimental module at the end of phase 2.
Secondly, once the main API is implemented, adding a
fetch()
wrapper on top of it is such a minuscule and trivial part of it, that's it's not even worth debating at this point.The reason I think it's worth mentioning as a general design goal, even if it's not as important as other aspects of the API (which is why it's in phase 4), is because it would be very convenient to use, similar to how
k6/http
is now. Most users won't care about configuring clients or changing transport-related settings, and would just want to fire off a quick request. In this sense, I expectfetch()
to be the most popular API, since it's familiar and easy to use. A good indicator of usable APIs is to expose complexity gradually and as needed; make things easy to use for newcomers, but flexible enough for people to explore naturally. To quote Alan Kay: "Simple things should be simple; complex things should be possible." ❤️ Since the overall goal of k6 is to deliver the best DX possible, having familiar and friendly wrappers like this aligns well with that mission.So please suggest which parts of this you would take out, or how you would change it, but I don't think we should remove the Fetch API as a design goal.
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.
I would argue the proposal here is about making things possible.
Both we and every user can (and will) extend this API.
But for me adding a bunch of UX improvements and that being not a small part of the specification does not help with the discussion around - it hinders it. I now or whoever has to read a bunch more text and then decide that whoever wrote it and the people who have agreed, meant those as inspirational things instead of as goals that need to be reached by this proposal to be called fulfilled.
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.
Would it help if we split this large proposal into several smaller ones? The scope is quite large already, and we haven't even fleshed out the details of the Sockets or DNS APIs. Splitting this into separate proposals would make each one more manageable, and allow us to prioritize them as a related group. Then the UX improvements, the Fetch API and any such convenience wrappers, could go into one.
I'm not sure where to start with this, so if we agree to do this, any suggestions to move it forward would be appreciated.
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.
At this point, I think we would be better served by proofs of concept. Iterating on code instead of on more lengthy text seems like it would be the more productive way forward.
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.
@imiric If possible, I would prefer to have one single source of truth and not fragment the information. In the end, the doc sounds still manageable to me.
I think the unique very detailed phase should be the first. All the rest should give us a guideline in terms of vision and roadmap for the long-term. We should re-iterate this doc at the end of each phase and before starting a new one.
I suggest changing phase 4 with something like 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.
Agreed. I started working on a PoC weeks ago, but Ned then suggested it was better to discuss the design first, so I abandoned it. At this point, it doesn't even feel like we agree on phase 1 of the current proposal, so I don't think we're anymore ready to work on the PoC than we were back then.
The reason to split this proposal into several more focused ones is to address Mihail's feedback that the scope of this proposal has expanded beyond just an HTTP API. And particularly to get rid of any mentions of UX improvements and the Fetch API, which seems to be controversial.
If we don't want to split it, then I guess everyone is fine with most of the sections here being incomplete, and the proposal "tiring to read"? It's frustrating trying to address some feedback, and then getting mixed signals about the way to proceed. 😓
In order to work on the PoC again, does everyone agree with the current phase 1?
That is: we won't be exposing a Sockets API, and network/transport options won't be configurable. The only goal is to expose a
Client.request()
that fixes a minor issue like #936 or #970. Although, in practice, both #936 and #970 will probably require configuring the transport, so maybe another issue would be a better fit.If you agree, please approve the PR and let's merge it as is. If not, please suggest improvements to phase 1 only. We can iterate on and flesh out the other phases later.
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.
If we implement fetch as a JS polyfill can we combine it in a native module? I guess we could have a wrapper on top of the new-k6-http-client+the polyfill. Am I missing something?
I think this is a nice UX and if we would not able to do it then we should insert it as a risk.
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.
My thinking so far is that the Fetch API would be part of the native Go module. There's really no reason it needs to be a JS library, and it would be a small addition anyway, so it shouldn't matter.
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.
Do we have an issue with it? It sounds like a prerequisite for the 3rd phase.
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 was my attempt at coming up with a File API, and is more of a thought experiment than anything else. #2977 goes into much more detail, and these kinds of issues should be addressed there.
This is inspired by Deno's API. While they do have sync versions of most APIs, it's clear that if we want to adopt streams, we'll need to defer actual reading of files to whatever process needs it. I.e. we can't read the whole file into memory here in the init context, so it's likely that this will open a file handle only, and
file.readable
will be the stream reference. Whether that can be done withoutawait
, or if we'll need to support this in the init context, is an open question.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.
I thought I commented this ... but :(
(top level await](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/await#top_level_await) is a ES2022 feature that made all the module resolutions in ESM asynchrnous. Which is partly what makes them a bit more involved and is what blocked them on async/await support in goja.
As soon as ESM is merged this will work as well. While I am in practice blocked specifically on fixing basically this functionality - I do need it for the basic implementation, so ... yeah - once ESM is native it will work.
2 workarounds are possible until then:
open
andrequire
are relative to? #2674