feat: sse-timing plugin for tracing events #1361
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1361 +/- ##
==========================================
+ Coverage 35.91% 43.58% +7.67%
==========================================
Files 69 76 +7
Lines 11576 12320 +744
==========================================
+ Hits 4157 5370 +1213
+ Misses 7104 6616 -488
- Partials 315 334 +19 |
|
@jizhuozhi Please fix the CI. |
|
cc @007gzs |
|
Please add e2e test cases. Reference https://github.com/alibaba/higress/tree/main/test/e2e/conformance/tests |
|
Will, I have seen the test-suits in https://github.com/alibaba/higress/tree/main/test/e2e/conformance/tests, it seems just supporting static response assertion as server-timing is streaming injecting dynamic metainfo into response body, and no sse server as upstream (I just found echo-server and echo-body in deployments). Does any solution or example here? |
| _num_headers: usize, | ||
| _end_of_stream: bool, | ||
| ) -> HeaderAction { | ||
| match self.get_http_response_header("Content-Type") { |
There was a problem hiding this comment.
The E2E test failed because it modifies the body; hence, it is necessary to remove the 'Content-Length' header during the header processing stage.
There was a problem hiding this comment.
According to text/event-stream MIME type, the origin server should not set Content-Length in response headers, I will change custom-response to support streaming response
There was a problem hiding this comment.
Using custom response to simulate responses will definitely add the content-length header. In real scenarios, when using the text/event-stream MIME type, it's not necessarily chunked transfer either; for example, if a proxy does caching in between, the content-length header might also be added. Therefore, the logic should still include compatibility handling and uniformly remove the content-length header.
| | headers | array of string | Optional | - | Custom HTTP response headers, keys and values separated by `=` | | ||
| | body | string | Optional | - | Custom HTTP response body | | ||
| | enable_on_status | array of number | Optional | - | Match original status codes to generate custom responses; if not specified, the original status code is not checked | | ||
| | is_stream | bool | Optional | false | If stream response, will not set Content-Length header | |
There was a problem hiding this comment.
In fact, it is not possible to control whether the content-length header is set, because when the plugin calls sendHTTPResponse, it will callback to the Envoy logic, which automatically adds this header
There was a problem hiding this comment.
As discussed above, the plugin needs to be compatible with the potential presence of the content-length response header, but modifying the body can lead to inconsistent lengths. Simply removing the content-length response header during the response header phase is sufficient, and Envoy will automatically switch to chunked transfer mode.
Ⅰ. Describe what this PR did
Add sse-timing plugin for tracing timestamp that each event received in higress. 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.
And implemention an EventStream for common SSE(text/event-stream) parsing.
Ⅱ. Does this pull request fix one issue?
None
Ⅲ. Why don't you add test cases (unit test/integration test)?
I'm sorry I don't know how to add WASM testcase, we could discuss in this PR.
Ⅳ. 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