Skip to content
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

sse: support Server Sent Event #3639

Closed
wants to merge 8 commits into from

Conversation

phymbert
Copy link

@phymbert phymbert commented Mar 13, 2024

What?

Server-Sent event http client support as an experimental module.

Why?

Server sent events are more and more used nowadays and I would love k6 to support it.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

Close #746

@phymbert phymbert requested a review from a team as a code owner March 13, 2024 09:30
@phymbert phymbert requested review from mstoykov and oleiade and removed request for a team March 13, 2024 09:30
@phymbert phymbert marked this pull request as draft March 13, 2024 09:30
@phymbert
Copy link
Author

@mstoykov @oleiade Hello, this is a first version, but I would be happy to receive your early feedback to guide me to get this feature implemented. Thanks

@oleiade
Copy link
Member

oleiade commented Mar 13, 2024

Bringing @szkiba to this discussion, too, as I know the dashboard feature is already built on top of SSE, and some code exists for supporting it there, too 🙇🏻

@phymbert phymbert marked this pull request as ready for review March 13, 2024 13:43
@phymbert
Copy link
Author

Bringing @szkiba to this discussion, too, as I know the dashboard feature is already built on top of SSE, and some code exists for supporting it there, too 🙇🏻

Could you please point me to this code ? I see nothing related to an sse k6 js module there. And #746 is opened since almost 6 years.

Meanwhile the PR is ready for review, it would be great if you can enable CI workflow to confirm all is OK. Thanks 👍

@oleiade
Copy link
Member

oleiade commented Mar 13, 2024

Hey @phymbert 👋🏻

To clarify, I was bringing @szkiba to the discussion as he might have good insights into the topic, as he's built a whole extension that relies on SSE to provide the dashboard feature. Not to question neither the rationale nor the quality of this PR 🙇🏻

The dashboard collects and aggregates metrics data from k6, and communicates them to the frontend part using SSE, I think the relevant files to look at for context are sse.go, aggregate.go, and web.go.

I've approved the CI to run 👍🏻

@phymbert
Copy link
Author

Thanks @oleiade for the CI run and clarification 🙇🏻. I understand failing checks are not related to these changes ? Please tell me if I have to adapt something.

Copy link
Contributor

@szkiba szkiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a rule, similar modules are first created as xk6 extensions. This way, anyone can use it and receive feedback. After that, if the topic is popular enough, the extension will become an experimental module. I would recommend this path here as well.

I would also consider using an SSE client library. There must be a compelling reason for the protocol implementation if there is a usable library (if it does not exist, then it should be justified why none of the existing libraries are suitable).

@phymbert
Copy link
Author

Rules like this are also a blocker for the adoption of the tool. That's probably why the issue has been created for 6 years. Users just consider SSE as not supported and drop k6 as an available solution.
I dont plan to support any k6 extension for 1,5k lines of code. SSE is just a custom lines break on top of HTTP 1.1, I always prefer to stick on the go sdk.

I do not see any understandable reason not to resolve the issue, adding a few lines of code, and letting people use SSE with k6.

If you have relevant feedback on the code, I will integrate them, or else I keep the PR opened if people need it in the future.


client.handleEvent("event", rt.ToValue(event))

case readErr := <-readErrChan:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Errors should also be measured with metrics, as in the http module. Instead of handling errors at the JavaScript level, it is much more convenient to set a threshold for the error metric...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of disagree on this - we do more or less the same for websockets. Especially as a lot of the cases it is just an error to close.

This still allows users to create custom metrics for errors, but doesn't force them.

We do have error metrics when teh connection couldn't be started though.

Also not SSE expert - so maybe not as similar 🤷

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I follow the approach from the websockets module as it was proposed in:

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The module will provide an `open` which will allow the user to pass a setup function to configure an `event` callback as it is done in the `ws` module with `message`.

### Limitation
The module will not support async io and the javascript main loop will be blocked during the http request duration.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Async support is becoming more and more important, so integrating a new module without async support is questionable. This is just my personal opinion, @mstoykov is more competent in this...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that SSE is by default an asynchronous communication I find that this must be asynchronous as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by asynchronous ? it's one way server to client data flow on top of HTTP 1.1 w/ single connection. The event loop is blocked while waiting for next event, but on each new event the client callback is triggered.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at xk6-mock extension code base, especially in the szkiba/muxpress library. You will find example there how to support callback running on eventloop.

var url = "https://echo.websocket.org/.sse";
var params = {"tags": {"my_tag": "hello"}};

var response = sse.open(url, params, function (client) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An SSE connection is usually long-lived. We do not test normal operation by opening a new connection in each iteration.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@szkiba
Copy link
Contributor

szkiba commented Mar 14, 2024

Rules like this are also a blocker for the adoption of the tool.

I rather see such rules as helping to keep the code base stable.

SSE is just a custom lines break on top of HTTP 1.1, I always prefer to stick on the go sdk.

go sdk is great i like it too. At the same time, I don't tend to reimplement every protocol, even the simplest ones, if there is a usable implementation.

I do not see any understandable reason not to resolve the issue, adding a few lines of code, and letting people use SSE with k6.

The opinion of the maintainers of the k6 code base really matters here, that's why there is a code review.

If you have relevant feedback on the code, I will integrate them, or else I keep the PR opened if people need it in the future.

I wrote a few comments, I don't know if they are relevant enough: metrics should be made about errors and not simply delegated to the JavaScript level, SSE comments should also be made available and not simply ignored, and async support should be considered.

@phymbert
Copy link
Author

@szkiba Thanks for your review, I will integrate the proposed changes.

go sdk is great i like it too. At the same time, I don't tend to reimplement every protocol, even the simplest ones, if there is a usable implementation.

Do you have some ? I see one here:

https://github.com/r3labs/sse/blob/c6d5381ee3ca63828b321c16baa008fd6c0b4564/client.go#L322-L335

IMHO we do not need to depend on a whole library for a small function, although the proposed version here must be improved (trailing new lines, empty data).

@mstoykov regarding async io, could you please assist me with a code to look at it ?

@oleiade
Copy link
Member

oleiade commented Mar 14, 2024

Hello, everyone 👋🏻

Firstly, I'd like to express our gratitude for the time and effort you've dedicated to implementing a missing and desirable feature for k6 with your proposal. We value contributions as such!

However, after discussions among the k6 maintainers' team, we've decided not to merge this PR in the core of k6 for several reasons outlined below, which aim to clarify our reasoning and rationale behind that decision.

1. HTTP module is implicitly frozen

We're currently focusing on developing a new http module. During this transitional period, we're limiting changes to the existing HTTP stack and feature-set to bug fixes. We acknowledge that we could have communicated this more effectively and publicly.

2. Extensions-first

As highlighted by @szkiba, this module is a perfect candidate for an xk6 extension: it allows for innovation, experimentation, and adding valuable APIs and features to k6 without impacting the stability of the core of k6, and relying on k6 code and constructs that are not accessible publicly (as far as I can tell).

It probably needs to be communicated better at the moment, but as @szkiba outlined, our position is that anything that can start or remain an extension should probably be. We likely want to support SSE indeed, but for this reason, as well as those depicted in 1. we would prefer to start having it as an extension. I'd be happy to provide support to get you started on that front should you want to do it too 🙇🏻 (Furthermore, we are also actively working on reducing the hurdles currently involved in using extensions in the future).

3. It's not just a few lines of code

Finally, the conversation around this PR has raised important points about the perceived simplicity of integrating new code versus the long-term commitments it entails.

I do not see any understandable reason not to resolve the issue, adding a few lines of code and letting people use SSE with k6.

And later:

I don't plan to support any k6 extension for 1,5k lines of code.

And the latter is a good illustration of the reason for the former. It highlights the perceived simplicity of integrating new code versus the long-term commitments it entails. While it may appear as a trivial change to do, and for us to "just" merge these 1.5k lines of code, doing so would delegate the support and maintenance of these lines of code to us.

We understand that this decision might be disappointing, but it's made with the best interest of the k6 open-source project and users in mind. k6 is a widely popular and adopted open-source project. Our goal and responsibility as maintainers is to ensure our software is easily maintainable and provides the best user experience, quality, and performance.

Next steps

Regarding the next steps, I sincerely think this is valuable work. I recommend making an extension out of it so that users who wish to use SSE can do so in the meantime (as we make the HTTP module ready to receive further updates). So we can learn more about its design, implementation, and performance aspects.

If extensions are not an option, another option would be to start a fork of k6 in the meantime. We make releases roughly every three months, rarely break APIs, and it would be a low-effort task.

As I mentioned earlier, I'd be eager to support you if you were to start an extension and pave the way for improvements to it if that's something you'd find useful and valuable.

@phymbert
Copy link
Author

Thanks for the clear explanation @oleiade , I understand all the point you raised.

I suggest the following approach - please tell me if I miss something:

  1. I finish the changes identified by @szkiba
  2. someone at GrafanaLabs creates a grafana/xk6-sse github repo
  3. I am pleased to submit the first PR with your help, you can PM me if needed
  4. close this pull request for archive
  5. following the interest of the community and with your support, you can count on me to maintain the xk6-sse in the future

Highly appreciated your kind feedback here 🙇🏻

@mstoykov
Copy link
Contributor

Thanks for the change, but I do agree with @szkiba and @oleiade on this being an extension. I will try to give some code feedback either way.

On top of the points by @oleiade, I will try to give more explanation on the topic of extensions and experimental modules.

Plugins Extensions

The whole idea of extensions (for the core team specifically) from the get go was allowing users of k6 to create modules (and later outputs) that aren't supposed to be maintained and scrutinized by the k6 core team.

This was particularly important when the team was smaller, but I would argue has become even more important as the scope has broadened.

This both allows users (you) to develop and use their extensions, without needing to care about whether anyone on the k6 core team cares whether their style is nice, the protocol should be implemented this way or async is important or not(async wasn't a thing back then - but you get the point). It allows you to make a single (potentially, not very working) things that fixes your immediate need.

And just as importantly lets you continue to iterate on it outside and without regards for the k6 release cycle or anything related with it. Potentially letting you break and improve the extension over time.

Also makes it possible (although not really relevant here) to implement

Experimental extensions modules

Experimental modules on the other hand are meant for things that the core team wants to be part of core more or less. But needs time to figure out what the API is and how it works with everything else.

We used to make extensions first and then import them - but this ... isn't ideal from development perspective. So for some new experimental modules we are skipping it.

But one of the important requirements is that the core team will want this in the core - usually without seeing big problems with it. Just because a module is experimental doesn't mean that we want to introduce breaking changes all the time or that it doesn't need to match the same quality that is in k6 core already.

As @oleiade said though - k6/http is in maintenance mode - we do make changes, but they usually need to be really small and (not or) really necessary.

Point in case the last 2 "features" that have been added are: asyncRequest which arguably makes asynchronous code useful and is a fairly small change, and support for metadata which arguably is not even k6/http specific at all.

@mstoykov
Copy link
Contributor

someone at GrafanaLabs creates a grafana/xk6-sse github repo

The actual idea is that you will create the repo and continue to maintain and own it.

Again big part of the extensions vs in core is about who owns and maintains the code. If it is a grafana repository we will now need to give you permission to commit and/or even worse review your commit and merge it without giving you any rights.

As pointed out - we are very unlikely to put this in core in the future. Any SSE solution will likely need to be based on http v2 api to be considered. And unfortunately that is blocked on ReadableStreams API.

@mstoykov regarding async io, could you please assist me with a code to look at it ?

This arguably is a lot to explain, but a good example of something similar is gRPC stream implementation. And in general the documentation on RegisterCallback. (I technically should write docs on this - it just always is not prioritized ... 😬 )

Also general knowledge about extensions creating can be found in the docs.

@phymbert
Copy link
Author

The actual idea is that you will create the repo and continue to maintain and own it.

This is not what I understood from @oleiade, having an extension on my own will ridicously reduced its visibility. And again, users will just go to another solution like Gatling which supports SSE very well out of the box.

If it is a grafana repository we will now need to give you permission to commit and/or even worse review your commit and merge it without giving you any rights.

Is it a problem to open a repo to other open-source contributor outside of GrafanaLabs ? is there any technical blocking point ? What are the difference with other extension like https://github.com/grafana/xk6-websockets ? You can as all open source project later on give Collaborator access to external users based on their contribution.

Thanks for the doc, I will read it out.

Simplify the event loop parsing, add spec link. Trim trailing spaces
@oleiade
Copy link
Member

oleiade commented Apr 4, 2024

Hi @phymbert 👋🏻

Thank you for your patience. We've taken some further time to discuss and evaluate your proposal internally.

Unfortunately, we're unable to offer hosting the extension as part of the Grafana GitHub organization. Therefore, our suggestion remains unchanged, and we recommend you create a dedicated xk6 extension repository.

Regarding visibility, if that is something you'd be able, allowed, and willing to do, we often feature user and community stories on the Grafana Labs blog. We'd be interested in exploring the possibility of collaborating on a blog post to highlight your work with k6 and how the development of this extension served your requirements. Would that be something of interest to you?

🙇🏻

@phymbert
Copy link
Author

phymbert commented Apr 4, 2024

Hi @oleiade and k6 team, thanks for your explanation, I understand.

I am happy to introduce the xk6-sse extension.

Feel free to contribute 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP SSE (Server-Sent Event) support for k6
4 participants