Add branching support for performance dashboard #749
Conversation
CaseyHillers
left a comment
There was a problem hiding this comment.
Angular Dart side LGTM, just a question on the updates to the time series model with branching.
Thanks for adding the test to the get benchmarks handler!
| (onZoomIn)="zoomInto = benchmark"> | ||
| </benchmark-card> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
nit: the project leaves a new line at EOF
| @KeyConverter() | ||
| Key get key => series.key; | ||
|
|
||
| Map<String, dynamic> get facade { |
There was a problem hiding this comment.
If SerializableTimeSeries is no longer used, we should move this into the model class to clean this up.
| 'ID': series.timeSeriesId, | ||
| 'Label': series.label, | ||
| 'TaskName': series.taskName, | ||
| 'Unit': series.unit, |
There was a problem hiding this comment.
nitty nit: I would add branch here and update the TimeSeriesValue.toJson() to not serialize it. Every single time series value shared will be repeating the same bytes. (I think it's fine to just add a TODO)
There was a problem hiding this comment.
I was thinking about that too. But it didn't work out based on current logic in https://github.com/flutter/cocoon/blob/master/app_dart/lib/src/request_handlers/get_benchmarks.dart
It queries recent 50 commits, and recent 50 TimeseriesValue, then if the sha matches between those two entities, a point is recorded, otherwise it will be noted as missing value.
When we consider branching, different branches may share same sha. Results will be messed up.
Another reason why I add the branch property to TimeseriesValue: existing logic queries data based on timestamp, which is the latest 50 records. If without branch property, there is no way to query the exact records of one specific branch. For example: in the latest 50 commits, if 48 is with master, 2 with release branch, the dashboard will show only the 2 records for that release branch, even though we may have older records beyond the 50 limit.
There was a problem hiding this comment.
I agree with the model needing to have the branch field for TimeSeriesValue. Only the backend needs to know about what branch a TimeSeriesValue is on. The frontend shouldn't need to further parse the data.
This was moreso on just hiding the TimeSeriesValue.branch field when returning the JSON.
There was a problem hiding this comment.
branch in TimeseriesValue is only as a Property, rather than a JsonKey:
So it should not be parsed in the frontend side.
|
|
||
| /// The branch of [commit]. | ||
| @StringProperty(propertyName: 'Branch') | ||
| String branch; |
There was a problem hiding this comment.
Is there a plan in place to update past time series data include the branch field? Or will this only work on new timeseries data?
There was a problem hiding this comment.
I have updated all historical data to have the branch property over the weekend.
| bool _isShowArchived = false; | ||
| bool _userIsAuthenticated = false; | ||
| List<String> _values = ['master']; | ||
| String _selectedValue = 'master'; |
There was a problem hiding this comment.
I think Uri.base.queryParameters['branch'] ?? 'master' would allow users to share the performance page of the branch they are viewing. set selectedValue(...) will have to be updated to set the query parameter.
There was a problem hiding this comment.
(This was a request on the build dashboard branch pages)
This reverts commit b7fbe86.
This PR is part of flutter/flutter#51807
It adds branch select on performance dashboard;
It adds branch parameter injection in get-benchmark API
It adds branch property in TimeseriesValue
It saves branch property when inserting records to TimeseriesValue