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

feat: implements text/event-stream(SSE) MIME parser #1416

Merged
merged 5 commits into from
Oct 24, 2024

Conversation

jizhuozhi
Copy link
Collaborator

Ⅰ. Describe what this PR did

Implements an EventStream for common SSE(text/event-stream) parsing.

And add sse-timing plugin for tracing timestamp that each event received in higress as an example. As an alternative implementation of the server-timing protocol (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Server-Timing). It's useful to profile AI backend latency which uses event-stream MIME type.

Ⅱ. Does this pull request fix one issue?

None

Ⅲ. Why don't you add test cases (unit test/integration test)?

As an example, there is no need to integration test, but has an unit test in event_stream.rs

Ⅳ. Describe how to verify it

There is a docker-compose.yml in plugin directory, with a go server backend which provides sse response, up it and then just use broswer to open http://localhost:10000, you could see events in dev-tools.

Ⅴ. Special notes for reviews

@jizhuozhi
Copy link
Collaborator Author

An alternative PR of #1361, but not as a publish plugin

@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.62%. Comparing base (ef31e09) to head (72c9fa9).
Report is 164 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1416      +/-   ##
==========================================
+ Coverage   35.91%   43.62%   +7.71%     
==========================================
  Files          69       76       +7     
  Lines       11576    12320     +744     
==========================================
+ Hits         4157     5375    +1218     
+ Misses       7104     6613     -491     
- Partials      315      332      +17     

see 69 files with indirect coverage changes

@johnlanni
Copy link
Collaborator

cc @007gzs

@jizhuozhi
Copy link
Collaborator Author

@johnlanni @007gzs PTAL

impl Context for SseTimingRoot {}

impl RootContext for SseTimingRoot {
fn on_configure(&mut self, _plugin_configuration_size: usize) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

使用的变量建议去掉开头下划线,找了下clippy 好像还没支持 rust-lang/rust-clippy#4239

plugins/wasm-rust/src/event_stream.rs Show resolved Hide resolved
Copy link
Collaborator

@007gzs 007gzs left a comment

Choose a reason for hiding this comment

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

LGTM

@007gzs 007gzs merged commit e7561c3 into alibaba:main Oct 24, 2024
12 checks passed
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.

4 participants