The UMA_HISTOGRAM_ENUMERATION macro [1] has two different forms of being called: one accepts a max-enum-value-plus-one as an argument and the other infers the max value it the provided enum has a kMaxValue value that maps to the max-enum-value. Given that difference, I found by chance one case [2] where the API is incorrectly used in offline pages code. We should review all other usages of that macro in the offline codebase [3] and fix other potential errors. [1] https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?q=UMA_HISTOGRAM_ENUMERATION&l=30-78 [2] https://cs.chromium.org/chromium/src/components/offline_pages/core/prefetch/prefetch_importer_impl.cc?l=31,50-51 [3] https://cs.chromium.org/search/?q=file:offline_pages+UMA_HISTOGRAM_ENUMERATION&sq=package:chromium&type=cs
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d6c4c54b5a6dbbe14e33e686e140d7d560799a03 commit d6c4c54b5a6dbbe14e33e686e140d7d560799a03 Author: Daniel Cheng <dcheng@chromium.org> Date: Tue Oct 16 08:39:46 2018 [offline pages] update histogram enums to use kMaxValue The two-argument version of UMA_HISTOGRAM_ENUMERATION can autodeduce the correct boundary value, so use that instead of defining a placeholder enumerator value that needs to be ignored in switch statements. Bug: 742517, 857134 Change-Id: I74ca2ace85fc6f3955f6f3a5e772e15bad8776e9 Reviewed-on: https://chromium-review.googlesource.com/c/1278060 Reviewed-by: Carlos Knippschild <carlosk@chromium.org> Reviewed-by: calamity <calamity@chromium.org> Commit-Queue: Daniel Cheng <dcheng@chromium.org> Cr-Commit-Position: refs/heads/master@{#599917} [modify] https://crrev.com/d6c4c54b5a6dbbe14e33e686e140d7d560799a03/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java [modify] https://crrev.com/d6c4c54b5a6dbbe14e33e686e140d7d560799a03/chrome/browser/offline_pages/android/offline_page_bridge.cc [modify] https://crrev.com/d6c4c54b5a6dbbe14e33e686e140d7d560799a03/chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc [modify] https://crrev.com/d6c4c54b5a6dbbe14e33e686e140d7d560799a03/components/offline_pages/core/client_namespace_constants.h [modify] https://crrev.com/d6c4c54b5a6dbbe14e33e686e140d7d560799a03/components/offline_pages/core/model/add_page_task_unittest.cc [modify] https://crrev.com/d6c4c54b5a6dbbe14e33e686e140d7d560799a03/components/offline_pages/core/model/clear_storage_task.h [modify] https://crrev.com/d6c4c54b5a6dbbe14e33e686e140d7d560799a03/components/offline_pages/core/model/delete_page_task_unittest.cc [modify] https://crrev.com/d6c4c54b5a6dbbe14e33e686e140d7d560799a03/components/offline_pages/core/model/mark_page_accessed_task.cc [modify] https://crrev.com/d6c4c54b5a6dbbe14e33e686e140d7d560799a03/components/offline_pages/core/model/offline_page_model_taskified.cc [modify] https://crrev.com/d6c4c54b5a6dbbe14e33e686e140d7d560799a03/components/offline_pages/core/offline_page_types.h [modify] https://crrev.com/d6c4c54b5a6dbbe14e33e686e140d7d560799a03/components/offline_pages/core/prefetch/prefetch_importer_impl.cc
I just want to make a note that a final cleanup step will be required to convert enums with mixed name patterns values to comply with kCamelCase.
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/75601d0b3031b08e8330369654425b773f473bd1 commit 75601d0b3031b08e8330369654425b773f473bd1 Author: Daniel Cheng <dcheng@chromium.org> Date: Wed Oct 17 14:24:35 2018 [offline pages] more UMA histogram cleanup - Update more scoped enums to use kMaxValue instead of defining a dummy COUNT enumerator. - Update more UMA enumeration histograms to use the two-argument version of the macro, which autodeduces the boundary value. - Use base::UmaHistogram* where appropriate instead of manually expanding the corresponding macros. Bug: 742517, 857134 Change-Id: I0feb837ecb41a780c8d3234e9d2e75c23229a24b Reviewed-on: https://chromium-review.googlesource.com/c/1283250 Commit-Queue: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Reviewed-by: Carlos Knippschild <carlosk@chromium.org> Cr-Commit-Position: refs/heads/master@{#600384} [modify] https://crrev.com/75601d0b3031b08e8330369654425b773f473bd1/components/offline_pages/core/background/offliner.h [modify] https://crrev.com/75601d0b3031b08e8330369654425b773f473bd1/components/offline_pages/core/background/request_coordinator.cc [modify] https://crrev.com/75601d0b3031b08e8330369654425b773f473bd1/components/offline_pages/core/background/request_coordinator_event_logger.cc [modify] https://crrev.com/75601d0b3031b08e8330369654425b773f473bd1/components/offline_pages/core/background/request_coordinator_event_logger_unittest.cc [modify] https://crrev.com/75601d0b3031b08e8330369654425b773f473bd1/components/offline_pages/core/background/request_coordinator_unittest.cc [modify] https://crrev.com/75601d0b3031b08e8330369654425b773f473bd1/components/offline_pages/core/background/request_notifier.h [modify] https://crrev.com/75601d0b3031b08e8330369654425b773f473bd1/components/offline_pages/core/model/persistent_page_consistency_check_task.cc [modify] https://crrev.com/75601d0b3031b08e8330369654425b773f473bd1/components/offline_pages/core/model/startup_maintenance_task.cc [modify] https://crrev.com/75601d0b3031b08e8330369654425b773f473bd1/components/offline_pages/core/offline_page_metadata_store.cc [modify] https://crrev.com/75601d0b3031b08e8330369654425b773f473bd1/components/offline_pages/core/offline_store_types.h [modify] https://crrev.com/75601d0b3031b08e8330369654425b773f473bd1/components/offline_pages/core/prefetch/prefetch_types.h [modify] https://crrev.com/75601d0b3031b08e8330369654425b773f473bd1/components/offline_pages/core/prefetch/store/prefetch_store.cc [modify] https://crrev.com/75601d0b3031b08e8330369654425b773f473bd1/components/offline_pages/core/prefetch/tasks/download_completed_task.cc [modify] https://crrev.com/75601d0b3031b08e8330369654425b773f473bd1/components/offline_pages/core/prefetch/tasks/prefetch_task_test_base_unittest.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/593985a892a32de785aad1688747363a0ff0e067 commit 593985a892a32de785aad1688747363a0ff0e067 Author: Carlos Knippschild <carlosk@chromium.org> Date: Wed Oct 31 22:03:50 2018 Add OfflineUsage metric alternative that is not offline resilient This change adds a new metrics that works as a mirror of OfflinePages.OfflineUsage, aptly named OfflinePages.OfflineUsage.NotOfflineResilient, that should report exactly the same values but does not accumulate counters in PrefsService, reporting them directly to UMA instead. We want to compare these two metrics to try and assess the loss bias of metrics often reported in offline situations. Bug: 899800, 857134 Change-Id: I90b83ce1da638ca389ef38785a9f5ef1bb4538f3 Reviewed-on: https://chromium-review.googlesource.com/c/1305397 Commit-Queue: Carlos Knippschild <carlosk@chromium.org> Reviewed-by: Ilya Sherman <isherman@chromium.org> Reviewed-by: Dmitry Titov <dimich@chromium.org> Cr-Commit-Position: refs/heads/master@{#604394} [modify] https://crrev.com/593985a892a32de785aad1688747363a0ff0e067/chrome/browser/offline_pages/prefetch/offline_metrics_collector_impl.cc [modify] https://crrev.com/593985a892a32de785aad1688747363a0ff0e067/chrome/browser/offline_pages/prefetch/offline_metrics_collector_impl.h [modify] https://crrev.com/593985a892a32de785aad1688747363a0ff0e067/chrome/browser/offline_pages/prefetch/offline_metrics_collector_impl_unittest.cc [modify] https://crrev.com/593985a892a32de785aad1688747363a0ff0e067/tools/metrics/histograms/histograms.xml
Comment 1 by petewil@chromium.org
, Jul 16Status: Available (was: Untriaged)