[app_dart] Add a handler that pushes benchmark results to Metrics Center #788

digiter · 2020-06-01T21:31:51Z

The handler reads benchmark results after [sinceMillis] parameter, transforms them into MetricPoints format, then writes to Metrics Center. The Metrics Center will transfer the data to Skia Perf later and let us evaluate the effectiveness of using Skia Perf.

Follow up PRs will add a cron job that runs this handler periodically.

Bug: flutter/flutter#58442

Currently the data store read operations of flutter-dashboard is about 906 millions per day.
This handler reads about 20 thousands data store records when transferring benchmark data for one day, so it's a tiny fraction of the overall usage.

godofredoc · 2020-06-02T16:25:03Z

app_dart/lib/src/datastore/cocoon_config.dart

Is this a new service account or an existing one? Let's create a doc to document the origin, purpose and location. Going forward we'll need to track very closely all the service accounts used in flutter infrastructure.

Good idea. Added a comment as the best source of truth.

godofredoc · 2020-06-02T16:25:45Z

app_dart/lib/src/request_handlers/push_benchmark_to_center.dart

Flutter? this should have failed the licenses check.

godofredoc · 2020-06-02T16:28:12Z

app_dart/lib/src/request_handlers/push_benchmark_to_center.dart

Don't we have a handler to push metrics? can we reuse the existing one instead of creating a new ad hoc handler?

See the comment below.

godofredoc · 2020-06-02T16:31:07Z

app_dart/lib/src/request_handlers/push_benchmark_to_center.dart

I'm a bit confused, why we need to read the benchmark from datastore instead of directly processing it from the origin. e.g the request payload.

Inserting to datastore and processing later is adding unnecessary jumps and complexity IMHO.

See the comment below.

We can directly translate the payload in the future. I think this is mainly to migrate old metrics from cocoon to Skia perf for historical checks.

Actually reusing this handler for data migration is what I think we should avoid. Data migration should be addressed separately with a plan that includes: validation of the data migration in the target, how much resources is going to use, estimated duration of the migration, rollback plan, etc.

The dashboard is a production app that the team relies on and we need to avoid unnecessary risks as much as possible. For migration I'd suggest you create a tool that runs separately from the dashboard and we have more control over the processing rate it will be using.

godofredoc · 2020-06-02T16:33:41Z

app_dart/lib/src/request_handlers/push_benchmark_to_center.dart

You can potentially move most of the content of this class to a service class and use it within the existing metrics handler to push the data. That way you won't need to synchronize timestamps and/or create an additional cronjob

The existing metrics handler only handles new data, @liyuqian and I would like to also transfer existing data to skia perf. Although we could export the existing data, however there's no easy way to process the exported data (leveldb log format).

Besides adding more burden to existing handler will increase its latency, complexity and decrease stability. For example, a failed pushing to metrics center could be caused by network interruptions temporarily, after the incident is over, there's no way to retry the pushing.

The existing metrics handler only handles new data, @liyuqian and I would like to also transfer existing data to skia perf. Although we could export the existing data, however there's no easy way to process the exported data (leveldb log format).

Besides adding more burden to existing handler will increase its latency, complexity and decrease stability. For example, a failed pushing to metrics center could be caused by network interruptions temporarily, after the incident is over, there's no way to retry the pushing.

If that is the case then I'd recommend to split the process in two parts one for the data migration and the other one for just sending the data as it arrives.

Data migration will require different processes, maybe running for days, data validation on the target, running after work hours to minimize disruption, etc.

For the second part the agent or the bots can call the two handlers with the same collected json.

This PR is partially implementing the data migration process.

For the second part, do you mean letting cocoon agent calls /api/push_benchmark_to_center in addition to /api/update-task-status?

BTW, @kf6gpe has a plan to have the new metrics co-exist in both SkiaPerf/MetricsCenter and Cocoon. That way, he can compare between two dashboards' regression alerts, and make sure that SkiaPerf's alerts are properly set before sunsetting the Cocoon dashboard.

This PR is partially implementing the data migration process.

For the second part, do you mean letting cocoon agent calls /api/push_benchmark_to_center in addition to /api/update-task-status?

Yes, that is correct.

godofredoc · 2020-06-02T16:34:07Z

app_dart/test/request_handlers/push_benchmark_to_center_test.dart

godofredoc · 2020-06-02T16:35:24Z

app_dart/test/request_handlers/push_benchmark_to_center_test.dart

Would parseSinceMillis make more sense in the utilities class? it would be easier to reuse and I don't think it depends on the handler.

The parameter is closely tied to the handler and I don't see the benefit from reusing it.

keyonghan · 2020-06-02T16:33:26Z

app_dart/lib/src/request_handlers/push_benchmark_to_center.dart

this condition seems not necessary: https://github.com/flutter/cocoon/blob/master/app_dart/lib/src/model/appengine/time_series.dart#L57

keyonghan · 2020-06-02T16:37:49Z

app_dart/lib/src/request_handlers/push_benchmark_to_center.dart

keyonghan · 2020-06-02T16:41:35Z

app_dart/lib/src/request_handlers/push_benchmark_to_center.dart

move these functions private if not intended to be called by others?

Files in lib/src have been library private already.

keyonghan · 2020-06-02T16:46:41Z

app_dart/test/src/datastore/fake_datastore.dart

No need to add this function. You can use values[key]=value to add value.

keyonghan · 2020-06-02T16:47:21Z

app_dart/test/request_handlers/push_benchmark_to_center_test.dart

see comment below.

keyonghan · 2020-06-02T16:50:24Z

app_dart/test/request_handlers/push_benchmark_to_center_test.dart

Test for destination result is missing.

keyonghan · 2020-06-02T16:51:12Z

app_dart/lib/src/request_handlers/push_benchmark_to_center.dart

It would be nice to add more doc. about Metrics center storage.

keyonghan · 2020-06-02T16:51:51Z

app_dart/pubspec.yaml

a tracking issue?

liyuqian · 2020-06-03T20:13:29Z

app_dart/lib/src/request_handlers/push_benchmark_to_center.dart

Maybe change "later than" to "strictly later than" to clarify that we're doing ">" instead of ">=" comparison.

liyuqian · 2020-06-03T20:21:27Z

app_dart/lib/src/request_handlers/push_benchmark_to_center.dart

Do you think we'll need a more sophisticated batch size limit like https://github.com/liyuqian/metrics_center/blob/master/lib/src/flutter/center.dart#L121 ? Notice that we're returning points that are strictly later than a timestamp, and we would miss some points if many points have the same timestamp, and we cut the batch in the middle.

…er#3.

Some part of this PR's code is from #788. Related issue: flutter/flutter#73872

godofredoc · 2021-06-03T22:09:18Z

Closing as outdated.

digiter requested review from godofredoc, keyonghan and liyuqian June 1, 2020 21:43

godofredoc reviewed Jun 2, 2020

View reviewed changes

keyonghan reviewed Jun 2, 2020

View reviewed changes

liyuqian reviewed Jun 3, 2020

View reviewed changes

digiter marked this pull request as draft June 17, 2020 21:01

digiter added 5 commits July 15, 2020 13:37

[app_dart] Add a handler for pushing benchmark to skia perf

1dfcdbc

temp transfer

165ad90

Fix name and sub_result data issue described in liyuqian/metrics_cent…

b0da2c9

…er#3.

merge upstream

0a3a172

Run twice per day.

503844f

liyuqian mentioned this pull request Jan 19, 2021

Add a script to manually transfer Cocoon metrics #1060

Merged

liyuqian added a commit that referenced this pull request Jan 21, 2021

Add a script to manually transfer Cocoon metrics (#1060)

4234ac4

Some part of this PR's code is from #788. Related issue: flutter/flutter#73872

godofredoc closed this Jun 3, 2021

[app_dart] Add a handler that pushes benchmark results to Metrics Center #788

[app_dart] Add a handler that pushes benchmark results to Metrics Center #788

Uh oh!

Conversation

digiter commented Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

digiter Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

digiter commented Jun 1, 2020 •

edited

Loading

digiter Jun 2, 2020 •

edited

Loading