Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2725913002:
[Catapult][Telemetry] Move trace_data from telemetry/ to tracing/ (Closed)
|
Created:
3 years, 9 months ago by rnephew (Reviews Here) Modified:
3 years, 9 months ago CC:
catapult-reviews_chromium.org Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[Catapult][Telemetry] Move trace_data from telemetry/ to tracing/
So that systrace can use it without depending on telemetry.
BUG=catapult:#3262
Review-Url: https://codereview.chromium.org/2725913002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/f94a852c0a3fbd1ebcbbb46889c83a217ca28b12
Patch Set 1 #Patch Set 2 : Move from common to tracing #
Total comments: 2
Messages
Total messages: 24 (15 generated)
The CQ bit was checked by rnephew@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rnephew@chromium.org changed reviewers: + charliea@chromium.org, nednguyen@google.com
After this lands, I will have a CL out that changes telemetry to use this one, and will change my current systrace CL to use this as well.
On 2017/03/01 18:49:05, rnephew (Reviews Here) wrote: > After this lands, I will have a CL out that changes telemetry to use this one, > and will change my current systrace CL to use this as well. I think we should move this into catapult/tracing/ instead
> I think we should move this into catapult/tracing/ instead I was told by someone (maybe charlie?) that catapult/tracing was all trace viewer code so was not the proper place for this. After the response to this I'll either keep it here or move it depending on what is said.
On 2017/03/01 19:02:01, rnephew (Reviews Here) wrote: > > I think we should move this into catapult/tracing/ instead > > I was told by someone (maybe charlie?) that catapult/tracing was all trace > viewer code so was not the proper place for this. After the response to this > I'll either keep it here or move it depending on what is said. Charlie: I think tracing/ is the best place for this, since it also includes code for slimming down the trace, merge the trace, computing metrics from traces,.. and not just trace_viewer.
The CQ bit was checked by rnephew@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/01 19:08:01, nednguyen wrote: > On 2017/03/01 19:02:01, rnephew (Reviews Here) wrote: > > > I think we should move this into catapult/tracing/ instead > > > > I was told by someone (maybe charlie?) that catapult/tracing was all trace > > viewer code so was not the proper place for this. After the response to this > > I'll either keep it here or move it depending on what is said. > > Charlie: I think tracing/ is the best place for this, since it also includes > code for slimming down the trace, merge the trace, computing metrics from > traces,.. and not just trace_viewer. I went ahead and just moved them to tracing/ since thats probably what will end up happening anyway.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Catapult][Telemetry] Move trace_data from telemetry/ to common/ So that systrace can use it without depending on telemetry. BUG=catapult:#3262 ========== to ========== [Catapult][Telemetry] Move trace_data from telemetry/ to tracing/ So that systrace can use it without depending on telemetry. BUG=catapult:#3262 ==========
lgtm This seems like a reasonable place to put it for me. https://codereview.chromium.org/2725913002/diff/20001/tracing/bin/run_py_tests File tracing/bin/run_py_tests (right): https://codereview.chromium.org/2725913002/diff/20001/tracing/bin/run_py_test... tracing/bin/run_py_tests:1: #!/usr/bin/env python Were trace_data and the unittest deleted from Telemetry in this CL? I only see stuff being added.
https://codereview.chromium.org/2725913002/diff/20001/tracing/bin/run_py_tests File tracing/bin/run_py_tests (right): https://codereview.chromium.org/2725913002/diff/20001/tracing/bin/run_py_test... tracing/bin/run_py_tests:1: #!/usr/bin/env python On 2017/03/03 22:03:54, charliea wrote: > Were trace_data and the unittest deleted from Telemetry in this CL? I only see > stuff being added. No. I will have a follow up CL that moves telemetry to use this and deletes the old version inside telemetry. Wanted to limit the scope of this CL to just the migration portion.
The CQ bit was checked by rnephew@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rnephew@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488580459960710,
"parent_rev": "3c626eaf72a7b8acde9fe17ec63b0b6d851a1a00", "commit_rev":
"f94a852c0a3fbd1ebcbbb46889c83a217ca28b12"}
Message was sent while issue was closed.
Description was changed from ========== [Catapult][Telemetry] Move trace_data from telemetry/ to tracing/ So that systrace can use it without depending on telemetry. BUG=catapult:#3262 ========== to ========== [Catapult][Telemetry] Move trace_data from telemetry/ to tracing/ So that systrace can use it without depending on telemetry. BUG=catapult:#3262 Review-Url: https://codereview.chromium.org/2725913002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
