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 5 commits
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.
I'm not a fan of these
dial*
functions. I added them inspired by Go'snet
package, but this doesn't provide the ability to implement custom dialers. So maybe we should expose someDialer
type? I wasn't sure how to structure it, though.And I'm slightly against calling these "dialers" to begin with, since it's mostly a Go concept. At the same time, I didn't want to call this
connect()
either, since it wouldn't make sense for UDP or IPC.dialIPC
still doesn't make sense to me, since it's essentially opening a file, but this too is following Go'snet
package. 🤔WDYT?
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 agree, what about trying to be consistent with the HTTP Client API and doing something similar?
We have a class, a constructor, and methods.
If it doesn't make sense for UDP then we can have
new UDP()
and that's all. This should also be very similar to the Linux API.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 had something like that initially, but decided against it, since you shouldn't be able to create a socket object, without it being actually connected/bound/open. Otherwise, you might be tempted to set some of its properties, which would be meaningless unless it's actually bound to an OS socket. In fact, most/all of its properties should be read-only. This is why I opted to have a constructor function instead.
An HTTP client is a bit different, since it's only used to make requests, and doesn't correspond to some system state. So you could theoretically have a Client instance that hasn't established a socket connection yet, and making the first request would establish the connection.
I also considered something like:
So in a sense you have some abstract protocol object, and calling
dial()
on it returns you aTCPSocket
,UDPSocket
orIPCSocket
instance. I'm slightly leaning towards this now, even thoughdial
is still in the name. WDYT?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.
Yeah, I'm also happy with it. The name
dial
is never mentioned in the RFC, so it could not be familiar to someone not experienced with Go. I thinkopen
orconnect
would be more readable (but it is very opinable) and more used in the JS ecosystemThere 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 went with
open
in 8c97fe0.connect
doesn't quite make sense for UDP or IPC, whereasopen
is generic enough to apply for all 3.I think
dial
is not just Go's invention. It does have some networking roots, though I'm not able to find definitive references outside of Go, and "dial-up modem" 😄, right now.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.
One more thing: the proposal doesn't currently specify a way to define custom dialers.
I wonder if we could handle this with an event system as well. E.g. have
before
/after
events scripts can subscribe to, similar to the HTTP events, emitted before/after the socket is opened.This could be a way to handle DNS lookups, instead of the currently proposed
lookup
hook function, but it would be generic enough for any other use cases (TCP proxies, custom metrics, etc.).So you would have something like:
I'm not entirely sold on the syntax to change
event.data
like this, as it's unclear which properties will be used after the event handler executes. Also, event handlers typically shouldn't return any values either, as that would make them too specific for a single purpose, so returning is not an option.I'm not really sure we should allow creating custom dialer implementations from scratch in JS. I.e. nobody would choose to implement custom protocols from scratch in JS, and if they need to do that, then our Go API should allow it by being easily extensible. So for JS, it might just be good enough to have hooks, or event handlers, so scripts can implement some custom logic around the socket implementation.
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 part of the current HTTP API is probably not a good design to copy. Why should a generic HTTP response have a
.json()
method?It's fine to have something that wraps a regular
Client
and works only with JSON, but a generichttp.Client
should probably not need to know what JSON is.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.
http.Client
wouldn't know what JSON is, but I don't see whyhttp.Response
couldn't have helper methods to parse the response body and return it as various objects.The web Fetch API has a
Response
with such methods, and so does Deno's implementation. Regardless if we end up implementing Fetch or not, this is very convenient, and it would be a UX regression if we decide to remove 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.
Fetch and Deno might have this, but Go, axios and got don't. We should consider what makes sense for us.
These are my thoughts on the topic, in roughly a descending order of certainty:
.json([selector])
method like the current one, where we have aselector
as the argument. That should be a separate JSONPath API that is not tied directly together to the core HTTP API, it should probably work with any string or buffer.Fetch
polyfill/wrapper that has a.json()
method, built on top of our generic HTTPClient
(either in Go or in JS!). This solves some of the UX problem of a generic API not having one.JSONClient
type built on top of the genericClient
, which will be well suited for easily working with JSON REST APIs. Including potentially automatically marshaling request bodies that are objects and maybe even automatically universalizing responses. Maybe even with some JSONPath integration 🤔 This provides all of the UX benefits, but without anything magical and with a clear separation of concerns (i.e. the composable approach).Client
should not make any guesses about the content of the HTTP requests and responses it is handling. This gives us more room for optimization, a clear separation of concerns, and a more consistent UX..json()
method (without any arguments) on the generic HTTPClient
. If someone really wants to use the generic k6 httpClient
for one-off request with a JSON response, thenJSON.parse(resp.body)
is not that much worse thanresp.json()
....json()
, are we also going to add to port the current.html()
? If not, then why not? And are also going to have.xml()
or.protobuf()
?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 wasn't familiar with got, looks nice 👍 It doesn't implement
json()
directly onResponse
, but does on thePromise
. It even supports thejson
property forPOST
requests.BTW, there's another HTTP lib from the same team called ky. 😅 This one apparently targets Deno, but it follows similar design principles as got.
Axios actually parses JSON bodies by default 😄 But I don't think we should look at it for inspiration, considering it's quite old at this point, and has been superseeded by other libs. It's still very popular and featureful, but also has many limitations.
Go shouldn't be an inspiration for the JS API. As stated in the design goals, we want to make things familiar to JS devs, not Go devs. We can borrow certain concepts to make the API composable and whatnot, but refering to it directly would only pollute our way of thinking about idiomatic JS.
If by "us" you mean "k6 users", then agreed. These convenience methods are all part of offering a good experience for JS developers. I don't see why that's so controversial.
To address some of your points:
Sure, the Selector API makes sense as a separate JSONPath API.
json()
should just return the body as an object, as it does in the other JS libs.Having purpose-built
Client
implementations just for sending different headers, interpreting requests and responses, seems like overkill, and might be too limiting in some cases. What if you want to send JSON but receive binary data, or any such weird combination we didn't predict? Having a singleClient
that offers some helper mechanisms for the most common use cases adds a fairly low overhead, which can be ignored for those who don't need it. I.e. everyone can just ignore thejson
property, and usebody
instead.I'm not sure why you keep mentioning this, but
Client
itself wouldn't have ajson()
method. It would return aPromise<Response>
which would have it.I don't see why we couldn't have
html()
either. The point of these methods are convenience for the most common use cases. If there are good reasons to includexml()
orprotobuf()
, then we should consider them as well.Are you suggesting that we have an
HTMLClient
, aProtobufClient
or anXMLClient
instead?Separate client implementations make sense for truly different protocols. So it probably makes sense to have a
GRPCClient
that extends the baseClient
, or maybe aSOAPClient
😄, but not just to avoid these simple payload handling helpers.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 mostly agree, but I will push back slightly on this. Go can and probably should be an inspiration, as long as the end result is also an idiomatic JavaScript API. If Go has an elegant solution to a problem, and if that solution doesn't look strange for JS APIs, is easy to understand, and cleanly solves a bunch of problems, why not adopt it? 😕 Sure, we don't need to copy the Go API directly, but we don't have to avoid its good ideas just be cause they are not JS. Good API design is somewhat universal after all...
I didn't meed completely separate
Client
implementations. Just wrappers around a genericClient
with maybe a few extra methods and some pre-defined Event handlers for the same events that you mention in the design doc.Use the generic
Client
, if it's a one-off, or build your own wrapper (because you should be able to easily do that, since everything is composable).Sure, but having more than one way to do the same thing, especially when it's a simple thing, is generally somewhat of an API design smell. I don't object too strongly to the simple
.json()
method, it's only at point 5 in my list, but I don't see the need. It's also one of these extra things that we can easily add at any future point, but once we add it we can never remove, so I don't see the need to start with it.Sorry, I meant the Response object returned by the generic Client (or, I guess, the Promise that resolves to it).
See the problems that
.html()
causes for the currentk6/http
API... Tightly coupling these is a mistake.No, that was the point. At best, we may have some helpers and wrappers, but we probably shouldn't. We should make it super easy and efficient for users to build whatever helpers and wrappers. We should provide the flexible and generic toolbox, not try to build a dedicated tool to solve every possible weird use case that exists out 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.
Hmm actually, I am slightly walking back some of my claims... 😅 Thinking about idiomatic
Promise
-based JS APIs, a.json()
helper on aPromise
that also returns anotherPromise
actually makes some sense 😅 I still don't like it, but it probably provides enough value to pass muster 🤔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.
Same logic as https://github.com/grafana/k6/pull/2971/files#r1161730930 - we shouldn't add any special handling of
json
options in the default Client. This is not composable, it's the opposite of that - trying to make aClient
that satisfies all use cases. And it adds "automagic behavior", as your// automatically adds 'Content-Type: application/json' header
comment directly shows.The request bodies that the normal HTTP Client accepts should probably be limited to
string
,ArrayBuffer
or some sort of a Stream. Anything else needs to be an error. We can have a separateRESTClient
orJSONClient
that has additional automated handling of JSON requests and responses.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 is not really "automagic" behavior, but a convenience for a very common use case. It's inspired by the request library, and was suggested in #436. The
json
property makes it clear what happens behind the scenes, and implicitly adding the header is much more convenient than having to remember to type it correctly manually, and usingJSON.stringify()
.I'll remove this if you feel strongly about it, but having separate client implementations just for this seems overkill.
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.
The problem is that it makes the API have weird edge cases. For example, what happens if I provide both
body
andjson
andformData
? 😅 Now the library needs to have some extra internal logic for the order of precedence, and we have do document and test that, etc. Having more than one way to do the same thing in the same object is generally not great API design.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.
Actually, from looking at the request library API you linked to, its
json
property is a boolean one, you still have to supply tobody
, right? Which may be even worse than this suggestion, since you now have extra parameters that just control the behavior of how thebody
is automagically processed.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.
Yes, it slightly complicates the validation, but passing conflicting options should just error out.
No,
json
can either be a boolean or an object.I don't like the boolean functionality either, but serializing the passed object and adding the header makes a lot of sense. This is also supported by got, as I mentioned in the other thread.
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 API proposal seems a bit messy. The HTTP request method should probably be part of the
Request
object, right? The whole point of aRequest
object is to fully contain everything you need to make a request.But then, what
client.get()
probably shouldn't acceptRequest
parameters, since thatRequest
could be aPOST
. I am not sure if methods like.get()
and.post()
are even necessary in the default HTTPClient
API 🤔 Maybe a single genericClient.request(Request)
orClient.do(Request)
is enough. But if we choose to have helper methods, they probably shouldn't accept aRequest
, just amethod, params, body
or something like that 🤔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 example is intentionally contrived, to show how request options can be applied globally and then merged/overridden, either in a
Request
object, or for each individualclient.get()
call.method
should indeed be part of theRequest
object. Essentially, most options that can be passed toclient.{get,request,...}
, can be used to construct aRequest
object. The point ofRequest
is to reuse common options to make multiple requests.I think overriding should be done from the bottom-up (or top-down, depending on how you look at it 😄). I.e. the options passed to
client.{get,request,...}
will override whatever was set inRequest
, which will in turn override whatever was set globally. So if you setmethod: 'POST'
in theRequest
, and you callclient.get()
with it, then aGET
request will be done, with whatever else was in theRequest
object.The same confusion could arise if we only allow
Client.request(Request)
. What should happen if you have:?
Removing the helper
.get()
and.post()
methods would just remove the convenience to not specifymethod
everytime, but not this possible inconsistency.Or would you want to remove all options from
client.request()
as well, and only allow passing aRequest
object? 🤔 I think forcing the use ofRequest
always would be inconvenient.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.