[L] Change in dart/sdk[main]: [dart:io] Add WebSocket timeline instrumentation(GSoC'26)
Samuel Rawlins (Gerrit)
Samuel Rawlins added 12 comments![]( "Open in Gerrit")
bool containsEvent(Not a hard requirement, but you could instead define a custom matcher. That would simplify the call sites to not always have `isTrue`, `isTrue`.
https://api.flutter.dev/flutter/package-matcher_matcher/CustomMatcher-class.html
It's not the simplest interface, but it does lead to very nice failure messages, and nice test expectations.
final args = event['args'];Sorry, a variable named `args` and one named `arguments` is too tricky for me to keep straight 😊. Let's name one `expectedArgs` and the other `actualArgs`.
continue;Can we throw an exception instead?
matches = false;I don't know if this matcher helper will get more complicated later, but we can just simplify this now down to `return false` here.
This might also be nicer if you write a CustomMatcher, because we can provide in the failure message which key/value didn't match.
expect(traceEvents.isNotEmpty, isTrue);We can use the isNotEmpty matcher: https://pub.dev/documentation/matcher/latest/matcher/isNotEmpty-constant.html
`expect(traceEvents, isNotEmpty);`
// Emits WebSocket timeline events.This should be a doc-comment, with `///`.
// Uses `connectionId` to correlate events for a given WebSocket connection andWith `///` comments, we can reference variables like this with `[]`: `[connectionId]`.
final int connectionId;Can this also be private? Even if you think it will be public later, we can make it private for now.
if (direction != null) 'direction': direction,We can use null-aware elements here: `'direction': ?direction`.
switch (opcode) {This could use a fancy switch expression:
```
return switch (opcode) {
_WebSocketOpcode.TEXT => 'text',
_WebSocketOpcode.BINARY => 'binary',
...
};
```
void Function /*Callback for reporting processed WebSocket frame metadata.*/ (Let's put this comment above the declaration here, before `void`.
// type, direction, size, and close information are recorded.Let's put the information kinds in quotes: 'type,' 'direction,' etc.
Related details
- Hangyu Jin
- Yash Hosalli
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yash Hosalli (Gerrit)
Yash Hosalli added 12 comments![]( "Open in Gerrit")
Not a hard requirement, but you could instead define a custom matcher. That would simplify the call sites to not always have `isTrue`, `isTrue`.
https://api.flutter.dev/flutter/package-matcher_matcher/CustomMatcher-class.html
It's not the simplest interface, but it does lead to very nice failure messages, and nice test expectations.
Done
Sorry, a variable named `args` and one named `arguments` is too tricky for me to keep straight 😊. Let's name one `expectedArgs` and the other `actualArgs`.
Did it..
Can we throw an exception instead?
Sure..
I don't know if this matcher helper will get more complicated later, but we can just simplify this now down to `return false` here.
This might also be nicer if you write a CustomMatcher, because we can provide in the failure message which key/value didn't match.
Sure...
We can use the isNotEmpty matcher: https://pub.dev/documentation/matcher/latest/matcher/isNotEmpty-constant.html
`expect(traceEvents, isNotEmpty);`
Done..
This should be a doc-comment, with `///`.
Ohh...done.
// Uses `connectionId` to correlate events for a given WebSocket connection andWith `///` comments, we can reference variables like this with `[]`: `[connectionId]`.
Aha...Thanks for this.
final int connectionId;Can this also be private? Even if you think it will be public later, we can make it private for now.
if you are pointing towards `_enabled` it is already private..
if you mean anything else... I didn't quite get it.
We can use null-aware elements here: `'direction': ?direction`.
Ya done...
This could use a fancy switch expression:
```
return switch (opcode) {
_WebSocketOpcode.TEXT => 'text',
_WebSocketOpcode.BINARY => 'binary',
...
};
```
Sure...
void Function /*Callback for reporting processed WebSocket frame metadata.*/ (Let's put this comment above the declaration here, before `void`.
Sure
// type, direction, size, and close information are recorded.Let's put the information kinds in quotes: 'type,' 'direction,' etc.
- Hangyu Jin
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yash Hosalli (Gerrit)
Yash Hosalli added 1 comment![]( "Open in Gerrit")
final int connectionId;Yash HosalliCan this also be private? Even if you think it will be public later, we can make it private for now.
if you are pointing towards `_enabled` it is already private..
if you mean anything else... I didn't quite get it.
Aha.. Sorry,
I realised you were pointing to `connectionId`.
made it private....
Related details
- Hangyu Jin
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted and added 3 comments![]( "Open in Gerrit")
Votes added by Samuel Rawlins
| Code-Review | +1 |
3 comments
// Returns true if the timeline contains an event with the given nameThis doc comment probably doesn't apply right to this class, could move it down to `containsTimelineEvent`.
Related details
- Ben Konyi
- Hangyu Jin
- Yash Hosalli
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yash Hosalli (Gerrit)
Yash Hosalli added 1 comment![]( "Open in Gerrit")
// Returns true if the timeline contains an event with the given nameThis doc comment probably doesn't apply right to this class, could move it down to `containsTimelineEvent`.
- Ben Konyi
- Hangyu Jin
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Moore (Gerrit)
Kevin Moore voted and added 4 comments![]( "Open in Gerrit")
Votes added by Kevin Moore
| Commit-Queue | +1 |
4 comments
I'm not sure if we document these types of changes in our changelog.
There is no new public API, but it might be nice to know that we have these events. I'd look through the current changelog to see if there is precedent!
cool to see this work!
(Note: I'm not an engineer. Please take my comments w/ high skepticism. Except for the spelling mistake. I'm pretty sure about that one!)
/// [_enabled] checks weather WebSocket timeline logging is currently enabled.Typo: `weather` should be `whether`.
webSocket._timelineLogger.logSend(opcode, data?.length ?? 0);If `_deflateHelper` is enabled, `data?.length` here represents the **compressed** payload size.
However, on the receiving side in `_messageFrameEnd`, the size logged is the **uncompressed** payload size (because `processIncomingMessage` is called before reporting the length).
This asymmetry could be confusing when analyzing timeline events. Consider logging the same metric (either compressed or uncompressed size) for both `Send` and `Receive` events.
});If `openUrl` or `request.close()` fails (e.g., due to a DNS resolution failure or a network error), the `Future` will fail and `connectTimeline?.finish()` will not be called, leaving the timeline event open.
Consider adding a `catchError` to handle these early connection failures and correctly finish the timeline task:
```dart
}).catchError((Object error, StackTrace stackTrace) {
connectTimeline?.finish(arguments: {'error': error.toString()});
return Future<WebSocket>.error(error, stackTrace);
});
```
or if this was an async function you could use try/finally which I think would be cleaner.
Related details
- Ben Konyi
- Hangyu Jin
- Samuel Rawlins
- Yash Hosalli
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yash Hosalli (Gerrit)
Yash Hosalli added 3 comments![]( "Open in Gerrit")
/// [_enabled] checks weather WebSocket timeline logging is currently enabled.Typo: `weather` should be `whether`.
Opps...
corrected it..
webSocket._timelineLogger.logSend(opcode, data?.length ?? 0);If `_deflateHelper` is enabled, `data?.length` here represents the **compressed** payload size.
However, on the receiving side in `_messageFrameEnd`, the size logged is the **uncompressed** payload size (because `processIncomingMessage` is called before reporting the length).
This asymmetry could be confusing when analyzing timeline events. Consider logging the same metric (either compressed or uncompressed size) for both `Send` and `Receive` events.
You are right.. I checked,
The situation becomes like:
send: compressed size
Receive: Uncompressed size
updated the send side instrumentation to record original size...
If `openUrl` or `request.close()` fails (e.g., due to a DNS resolution failure or a network error), the `Future` will fail and `connectTimeline?.finish()` will not be called, leaving the timeline event open.
Consider adding a `catchError` to handle these early connection failures and correctly finish the timeline task:
```dart
}).catchError((Object error, StackTrace stackTrace) {
connectTimeline?.finish(arguments: {'error': error.toString()});
return Future<WebSocket>.error(error, stackTrace);
});
```or if this was an async function you could use try/finally which I think would be cleaner.
You were right here too..
As suggested i have added a `catchError...`
Related details
- Ben Konyi
- Hangyu Jin
- Kevin Moore
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yash Hosalli (Gerrit)
Yash Hosalli added 1 comment![]( "Open in Gerrit")
I'm not sure if we document these types of changes in our changelog.
There is no new public API, but it might be nice to know that we have these events. I'd look through the current changelog to see if there is precedent!
cool to see this work!
(Note: I'm not an engineer. Please take my comments w/ high skepticism. Except for the spelling mistake. I'm pretty sure about that one!)
Thanks for the the improvement suggestions...
I will too check for any precedents available in changelog and circle back here..
Ben Konyi (Gerrit)
Ben Konyi added 8 comments![]( "Open in Gerrit")
First review pass. Thanks for digging into this! 😊
'connectionId': _connectionId,Nit: I'd create constants for each of these argument keys. In general, if we're using a string literal more than once, we should create an appropriately scoped constant to reduce the likelihood of typos and improve readability.
void logPing(int bytes, String direction) {Nit: I'd define an enum for the direction
```
enum _WebSocketTrafficDirection {
in,
out
}
```
And then you could just use `direction.name` when building the map.
_WebSocketOpcode.TEXT => 'text',It would be great if we could change `_WebSocketOpcode` to be an `enum` instead. That way, we could associate the opcode directly with the name and not have to pass around integers.
It'd be as simple as:
```dart
enum _WebSocketOpcode {
continuation(0),
text(1),
binary(2),
reserved3(3),
// ...
reservedF(15);
/// The integer representation of the op-code.
final int code;
const _WebSocketOpcode(this.code);
}
```
And then you'd be able to get the name from the enum directly, like:
```
_WebSocketOpcode.text.name
```
However, that might be worth doing in a follow up CL since it would involve some minor refactoring in this library.
TimelineTask? connectTimeline;I feel like this logic should be added to `_WebSocketTimelineLogger` instead so we can keep all of the timeline object management in a single class.
if (httpStatusCode != null) 'httpStatusCode': httpStatusCode,Nit: this can be
```dart
'httpStatusCode': ?httpStatusCode,
```
transformer.onFrame = (opcode, bytes, closeCode, closeReason) {I'm not sure it makes sense to introduce this indirection here. Is there any reason we can't just inject `_timelineLogger` into the transformer and unconditionally invoke these methods directly? The `log*` methods already have logic to check if the timeline logging is enabled, so using `onFrame != null` to indicate we should log isn't necessary.
Related details
- Hangyu Jin
- Kevin Moore
- Samuel Rawlins
- Yash Hosalli
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yash Hosalli (Gerrit)
Yash Hosalli added 6 comments![]( "Open in Gerrit")
Nit: I'd create constants for each of these argument keys. In general, if we're using a string literal more than once, we should create an appropriately scoped constant to reduce the likelihood of typos and improve readability.
Sure..
Nit: I'd define an enum for the direction
```
enum _WebSocketTrafficDirection {
in,
out
}
```And then you could just use `direction.name` when building the map.
Done...
I feel like this logic should be added to `_WebSocketTimelineLogger` instead so we can keep all of the timeline object management in a single class.
I Have migrated it too _WebSocketTimelineLogger..
if (httpStatusCode != null) 'httpStatusCode': httpStatusCode,Nit: this can be
```dart
'httpStatusCode': ?httpStatusCode,
```
Yes....Sure
transformer.onFrame = (opcode, bytes, closeCode, closeReason) {I'm not sure it makes sense to introduce this indirection here. Is there any reason we can't just inject `_timelineLogger` into the transformer and unconditionally invoke these methods directly? The `log*` methods already have logic to check if the timeline logging is enabled, so using `onFrame != null` to indicate we should log isn't necessary.
Ya correct..
I've updated the implementation to inject `_timelineLogger` directly into `_WebSocketProtocolTransformer` and removed the `onFrame` callback indirection entirely.
The transformer now emits timeline events directly via `_timelineLogger`
These `break`s aren't necessary.
- Ben Konyi
- Hangyu Jin
- Kevin Moore
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yash Hosalli (Gerrit)
Yash Hosalli added 1 comment![]( "Open in Gerrit")
First review pass. Thanks for digging into this! 😊
Thanks for the review....😊