Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1765293002:
Add support for Blimp engine metrics reporting in development. (Closed)
DescriptionAdd support for Blimp engine metrics reporting in development.
Currently there is no way of recording or reporting metrics for the Blimp engine.
To collect metrics for the Blimp engine in development two things are required. First, for metrics recording and reporting to be enabled on the Blimp engine. Second, for the metrics logging to flag Blimp engine reports so they can be filtered from other linux metrics reports. These code behavior changes are currently kept between a single build arg, but are controlled via separate defines checks.
These changes will allow metrics to be collected on development instances of Blimp and viewed separately from Linux builds.
BUG=592757
Committed: https://crrev.com/ab7a08b61e58d1a50565cb892f3402cebafbcf16
Cr-Commit-Position: refs/heads/master@{#380760}
Patch Set 1 : Create args #Patch Set 2 : Update arg location so check only occurs if was declared. #
Total comments: 6
Patch Set 3 : Add comment on new behavior. What, why, and where set in build. #
Total comments: 2
Patch Set 4 : Comment why not including the build logic in the corresponding gyp file. #Patch Set 5 : Create instructions and guidelines on creating new build arguments for blimp. #Patch Set 6 : Rollback. Committed in wrong branch. #
Total comments: 2
Patch Set 7 : Rename #define. Replace HEADLESS with BLIMP to clarify use. #
Messages
Total messages: 32 (13 generated)
Description was changed from ========== Add support for Blimp engine metrices reporting in development. Currently there is no way of recording or reporting metrics for the Blimp engine. To collect metrics for the Blimp engine in development two things are required. First, for metrics recording and reporting to be enabled on the Blimp engine. Second, for the metrics logging to flag Blimp engine reports so they can be filtered from other linux metrics reports. These code behavior changes are currently kept between a single build arg, but are controlled via separate defines checks. These changes will allow metrics to be collected on development instances of Blimp and viewed separately from Linux builds. BUG= ========== to ========== Add support for Blimp engine metrics reporting in development. Currently there is no way of recording or reporting metrics for the Blimp engine. To collect metrics for the Blimp engine in development two things are required. First, for metrics recording and reporting to be enabled on the Blimp engine. Second, for the metrics logging to flag Blimp engine reports so they can be filtered from other linux metrics reports. These code behavior changes are currently kept between a single build arg, but are controlled via separate defines checks. These changes will allow metrics to be collected on development instances of Blimp and viewed separately from Linux builds. BUG= ==========
jessicag@chromium.org changed reviewers: + asvitkine@chromium.org, dpranke@chromium.org
jessicag@chromium.org changed required reviewers: + asvitkine@chromium.org, dpranke@chromium.org
Please let me know if I am off on any best practices or if you have recommendations on who to loop in as an OWNER on the remaining file.
//build change lgtm.
jessicag@chromium.org changed reviewers: + brettw@chromium.org
jessicag@chromium.org changed required reviewers: + brettw@chromium.org
+brettw for chrome/browser/browser_process_impl.cc
https://codereview.chromium.org/1765293002/diff/20001/chrome/browser/browser_... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/1765293002/diff/20001/chrome/browser/browser_... chrome/browser/browser_process_impl.cc:857: #if defined(ENABLE_REPORTING_HEADLESS) Please add a comment about what this is and point at the build file that defines this. https://codereview.chromium.org/1765293002/diff/20001/components/metrics/BUIL... File components/metrics/BUILD.gn (right): https://codereview.chromium.org/1765293002/diff/20001/components/metrics/BUIL... components/metrics/BUILD.gn:361: } Do these same changes need to be done at the gyp level somewhere, or has the build system completely migrated to GN at this point?
https://codereview.chromium.org/1765293002/diff/20001/chrome/browser/browser_... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/1765293002/diff/20001/chrome/browser/browser_... chrome/browser/browser_process_impl.cc:857: #if defined(ENABLE_REPORTING_HEADLESS) On 2016/03/07 20:38:25, Alexei Svitkine (slow) wrote: > Please add a comment about what this is and point at the build file that defines > this. Done. PTAL. https://codereview.chromium.org/1765293002/diff/20001/components/metrics/BUIL... File components/metrics/BUILD.gn (right): https://codereview.chromium.org/1765293002/diff/20001/components/metrics/BUIL... components/metrics/BUILD.gn:361: } On 2016/03/07 20:38:25, Alexei Svitkine (slow) wrote: > Do these same changes need to be done at the gyp level somewhere, or has the > build system completely migrated to GN at this point? Blimp builds are entirely GN based and, for our use, gyp is entirely deprecated. This arg should NOT be used outside blimp, but let me know if you'd prefer for it to also appear in the gyp for the sake of consistency.
Please file a crbug for this and associate it with this change. https://codereview.chromium.org/1765293002/diff/20001/components/metrics/BUIL... File components/metrics/BUILD.gn (right): https://codereview.chromium.org/1765293002/diff/20001/components/metrics/BUIL... components/metrics/BUILD.gn:361: } On 2016/03/07 21:09:21, Jess wrote: > On 2016/03/07 20:38:25, Alexei Svitkine (slow) wrote: > > Do these same changes need to be done at the gyp level somewhere, or has the > > build system completely migrated to GN at this point? > > Blimp builds are entirely GN based and, for our use, gyp is entirely deprecated. > This arg should NOT be used outside blimp, but let me know if you'd prefer for > it to also appear in the gyp for the sake of consistency. Ah OK. Fine to keep it just in the GN file. Please add a comment above the block about it, so it's clear to (someone like me who didn't know the above) that it's not an accidental omission. https://codereview.chromium.org/1765293002/diff/40001/components/metrics/metr... File components/metrics/metrics_log.cc (right): https://codereview.chromium.org/1765293002/diff/40001/components/metrics/metr... components/metrics/metrics_log.cc:329: os->set_name("Blimp"); Note: That this will require server-side changes to support it. Happy to assist you in making those changes (I think I pointed at the relevant code snippet we have on the doc where we discussed it - so feel free to change that and send me the CL.)
lgtm % the above
Description was changed from ========== Add support for Blimp engine metrics reporting in development. Currently there is no way of recording or reporting metrics for the Blimp engine. To collect metrics for the Blimp engine in development two things are required. First, for metrics recording and reporting to be enabled on the Blimp engine. Second, for the metrics logging to flag Blimp engine reports so they can be filtered from other linux metrics reports. These code behavior changes are currently kept between a single build arg, but are controlled via separate defines checks. These changes will allow metrics to be collected on development instances of Blimp and viewed separately from Linux builds. BUG= ========== to ========== Add support for Blimp engine metrics reporting in development. Currently there is no way of recording or reporting metrics for the Blimp engine. To collect metrics for the Blimp engine in development two things are required. First, for metrics recording and reporting to be enabled on the Blimp engine. Second, for the metrics logging to flag Blimp engine reports so they can be filtered from other linux metrics reports. These code behavior changes are currently kept between a single build arg, but are controlled via separate defines checks. These changes will allow metrics to be collected on development instances of Blimp and viewed separately from Linux builds. BUG=592757 ==========
https://codereview.chromium.org/1765293002/diff/20001/components/metrics/BUIL... File components/metrics/BUILD.gn (right): https://codereview.chromium.org/1765293002/diff/20001/components/metrics/BUIL... components/metrics/BUILD.gn:361: } On 2016/03/07 21:17:44, Alexei Svitkine (slow) wrote: > On 2016/03/07 21:09:21, Jess wrote: > > On 2016/03/07 20:38:25, Alexei Svitkine (slow) wrote: > > > Do these same changes need to be done at the gyp level somewhere, or has the > > > build system completely migrated to GN at this point? > > > > Blimp builds are entirely GN based and, for our use, gyp is entirely > deprecated. > > This arg should NOT be used outside blimp, but let me know if you'd prefer for > > it to also appear in the gyp for the sake of consistency. > > Ah OK. Fine to keep it just in the GN file. Please add a comment above the block > about it, so it's clear to (someone like me who didn't know the above) that it's > not an accidental omission. Done. https://codereview.chromium.org/1765293002/diff/40001/components/metrics/metr... File components/metrics/metrics_log.cc (right): https://codereview.chromium.org/1765293002/diff/40001/components/metrics/metr... components/metrics/metrics_log.cc:329: os->set_name("Blimp"); On 2016/03/07 21:17:44, Alexei Svitkine (slow) wrote: > Note: That this will require server-side changes to support it. Happy to assist > you in making those changes (I think I pointed at the relevant code snippet we > have on the doc where we discussed it - so feel free to change that and send me > the CL.) Acknowledged.
maniscalco@chromium.org changed reviewers: + maniscalco@chromium.org
https://codereview.chromium.org/1765293002/diff/100001/chrome/browser/browser... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/1765293002/diff/100001/chrome/browser/browser... chrome/browser/browser_process_impl.cc:857: #if defined(ENABLE_REPORTING_HEADLESS) Was leaving the word blimp out of the #define an explicit decision? I'm wondering because we have this is_headless GN arg. If the is_headless stuff started using ENABLE_REPORTING_HEADLESS would that be a good thing or bad thing? In other words, I'm wondering if this is the #define for just blimp or for all headless things that report UMA.
Hmm, I feel like maybe we should add BLIMP to that define - otherwise it's hard to reason about what are all these headless things that would be using it going forward. On Tue, Mar 8, 2016 at 11:38 AM, <maniscalco@chromium.org> wrote: > > > https://codereview.chromium.org/1765293002/diff/100001/chrome/browser/browser... > File chrome/browser/browser_process_impl.cc (right): > > > https://codereview.chromium.org/1765293002/diff/100001/chrome/browser/browser... > chrome/browser/browser_process_impl.cc:857: #if > defined(ENABLE_REPORTING_HEADLESS) > Was leaving the word blimp out of the #define an explicit decision? I'm > wondering because we have this is_headless GN arg. If the is_headless > stuff started using ENABLE_REPORTING_HEADLESS would that be a good thing > or bad thing? In other words, I'm wondering if this is the #define for > just blimp or for all headless things that report UMA. > > https://codereview.chromium.org/1765293002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/03/08 17:28:06, Alexei Svitkine (slow) wrote: > Hmm, I feel like maybe we should add BLIMP to that define - otherwise it's > hard to reason about what are all these headless things that would be using > it going forward. I agree, particularly since there's already a separate headless chrome project in progress. If that's not going to use this flag, then this is a bad name. -- Dirk
Agreed and changed. https://codereview.chromium.org/1765293002/diff/100001/chrome/browser/browser... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/1765293002/diff/100001/chrome/browser/browser... chrome/browser/browser_process_impl.cc:857: #if defined(ENABLE_REPORTING_HEADLESS) On 2016/03/08 16:38:22, maniscalco wrote: > Was leaving the word blimp out of the #define an explicit decision? I'm > wondering because we have this is_headless GN arg. If the is_headless stuff > started using ENABLE_REPORTING_HEADLESS would that be a good thing or bad thing? > In other words, I'm wondering if this is the #define for just blimp or for all > headless things that report UMA. Good point. I think I was trying to use to variable naming to explain why it would be used. Given the added comment it doesn't make much sense and could easily be confused with is headless. Renamed.
lgtm
lgtm
The CQ bit was checked by jessicag@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/1765293002/#ps120001 (title: "Rename #define. Replace HEADLESS with BLIMP to clarify use.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1765293002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1765293002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jessicag@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1765293002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1765293002/120001
Message was sent while issue was closed.
Description was changed from ========== Add support for Blimp engine metrics reporting in development. Currently there is no way of recording or reporting metrics for the Blimp engine. To collect metrics for the Blimp engine in development two things are required. First, for metrics recording and reporting to be enabled on the Blimp engine. Second, for the metrics logging to flag Blimp engine reports so they can be filtered from other linux metrics reports. These code behavior changes are currently kept between a single build arg, but are controlled via separate defines checks. These changes will allow metrics to be collected on development instances of Blimp and viewed separately from Linux builds. BUG=592757 ========== to ========== Add support for Blimp engine metrics reporting in development. Currently there is no way of recording or reporting metrics for the Blimp engine. To collect metrics for the Blimp engine in development two things are required. First, for metrics recording and reporting to be enabled on the Blimp engine. Second, for the metrics logging to flag Blimp engine reports so they can be filtered from other linux metrics reports. These code behavior changes are currently kept between a single build arg, but are controlled via separate defines checks. These changes will allow metrics to be collected on development instances of Blimp and viewed separately from Linux builds. BUG=592757 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add support for Blimp engine metrics reporting in development. Currently there is no way of recording or reporting metrics for the Blimp engine. To collect metrics for the Blimp engine in development two things are required. First, for metrics recording and reporting to be enabled on the Blimp engine. Second, for the metrics logging to flag Blimp engine reports so they can be filtered from other linux metrics reports. These code behavior changes are currently kept between a single build arg, but are controlled via separate defines checks. These changes will allow metrics to be collected on development instances of Blimp and viewed separately from Linux builds. BUG=592757 ========== to ========== Add support for Blimp engine metrics reporting in development. Currently there is no way of recording or reporting metrics for the Blimp engine. To collect metrics for the Blimp engine in development two things are required. First, for metrics recording and reporting to be enabled on the Blimp engine. Second, for the metrics logging to flag Blimp engine reports so they can be filtered from other linux metrics reports. These code behavior changes are currently kept between a single build arg, but are controlled via separate defines checks. These changes will allow metrics to be collected on development instances of Blimp and viewed separately from Linux builds. BUG=592757 Committed: https://crrev.com/ab7a08b61e58d1a50565cb892f3402cebafbcf16 Cr-Commit-Position: refs/heads/master@{#380760} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ab7a08b61e58d1a50565cb892f3402cebafbcf16 Cr-Commit-Position: refs/heads/master@{#380760} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
