Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
862723002:
leveldb: Moved to LevelDBEnv.IOError.BFE histogram entries. (Closed)
Descriptionleveldb: Moved to LevelDBEnv.IOError.BFE histogram entries.
When Chrome's leveldb Env (ChromiumEnv) switched to using base::File for I/O, in
crrev.com/710373002, it also (unintentionally) switched some logged histogram
values from errno to base::File::Error. Prior to r305020 some values were logged
with errno values, others with PlatformFileError values, and still others with
Windows Error values (i.e. GetLastError()). The Windows values were introduced
in r245135 - also uintentionally. Teasing these apart is too difficult now so
switching to a new set of histograms reporting base::File::Error's.
BUG=431914
Committed: https://crrev.com/58023982f5703b46008206f83f534d45dfd481de
Cr-Commit-Position: refs/heads/master@{#322054}
Patch Set 1 #
Total comments: 6
Patch Set 2 : BFE -> PFE, combined histograms, & obsoleted some others. #Patch Set 3 : Rebased on jsbell's change to remove errno parsing. #
Total comments: 19
Patch Set 4 : Added back in IndexedDBLevelDBPFEMethods, fixed dates, misc. #
Total comments: 4
Patch Set 5 : Fixed several obsolete messages to refer to correct histograms. #Patch Set 6 : Rebased on ToT (to very pretty histograms.xml) - no changes. #
Messages
Total messages: 32 (11 generated)
cmumford@chromium.org changed reviewers: + dgrogan@chromium.org, jsbell@chromium.org
dgrogan for review, jsbell for FYI Here is a second start at fixing the IDB histogram values. This approach moves all LevelDBEnv.IDB.IOError.* entries to LevelDBEnv.IDB.IOError.PFE.*, and all LevelDBEnv.IOError.* to LevelDBEnv.IOError.PFE.* This review only adds a description for the NewLogger value, the rest would follow (as well as obsoleting the non-PFE values) if this approach was deemed correct.
The approach of starting new histograms to replace the old and slightly borked ones is good. https://codereview.chromium.org/862723002/diff/1/third_party/leveldatabase/en... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/862723002/diff/1/third_party/leveldatabase/en... third_party/leveldatabase/env_chromium.cc:534: uma_ioerror_base_name_ = name_ + ".IOError.PFE"; maybe use bFE to indicate base::File::Error instead of the old name PlatformFileError? https://codereview.chromium.org/862723002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/862723002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:11785: +<histogram name="LevelDBEnv.IOError.PFE.NewLogger" enum="PlatformFileError"> Maybe I misunderstood your disclaimer about the incompleteness of this patch but you don't need this entry (and should delete it) because it is taken care of by your new histogram_suffixes below. https://codereview.chromium.org/862723002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:59885: + <affected-histogram name="LevelDBEnv.IOError.PFE"/> What about the 3 affected histograms currently in IndexedDBLevelDBPFEMethods? Looks like perhaps this new histogram_suffixes entry is replacing the existing LevelDBEnvPlatformFileErrors entry, not the existing IndexedDBLevelDBPFEMethods entry. Maybe they can be combined into one? And now that I'm reminded of the existence of the WebCore.IndexedDB.LevelDB{Open,Read,Write}Errors.Errno.* histograms, are they affected similarly?
https://codereview.chromium.org/862723002/diff/1/third_party/leveldatabase/en... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/862723002/diff/1/third_party/leveldatabase/en... third_party/leveldatabase/env_chromium.cc:534: uma_ioerror_base_name_ = name_ + ".IOError.PFE"; On 2015/01/21 01:47:56, dgrogan wrote: > maybe use bFE to indicate base::File::Error instead of the old name > PlatformFileError? I started with that originally, but eventually got up into histograms.xml where I specified the enum to be PlatformFileError - which as you mentioned is really no longer. I felt it was better to be consistently wrong with the name. I'll change it to "BFE" and let's see how it looks. https://codereview.chromium.org/862723002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/862723002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:11785: +<histogram name="LevelDBEnv.IOError.PFE.NewLogger" enum="PlatformFileError"> On 2015/01/21 01:47:56, dgrogan wrote: > Maybe I misunderstood your disclaimer about the incompleteness of this patch but > you don't need this entry (and should delete it) because it is taken care of by > your new histogram_suffixes below. Yes (as I understand it) I will get a generic one for all members of IndexedDBLevelDBPFEMethodsNew right? https://codereview.chromium.org/862723002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:59885: + <affected-histogram name="LevelDBEnv.IOError.PFE"/> On 2015/01/21 01:47:56, dgrogan wrote: > What about the 3 affected histograms currently in IndexedDBLevelDBPFEMethods? > Looks like perhaps this new histogram_suffixes entry is replacing the existing > LevelDBEnvPlatformFileErrors entry, not the existing IndexedDBLevelDBPFEMethods > entry. Maybe they can be combined into one? Yes, I think they can be. I combined them - let me know if that won't work. > And now that I'm reminded of the existence of the > WebCore.IndexedDB.LevelDB{Open,Read,Write}Errors.Errno.* histograms, are they > affected similarly? The WebCore.IndexedDB.LevelDB{Open,Read,Write}Errors.Errno.* are now all obsolete (as of r305020 since we don't log those anymore). I'll be taking out the Error stuff from the code, but this CL is already too complex. This CL has marked all the *.Errno.* errors as obsolete.
No new changes - just rebased on jsbell's changes to remove errno parsing.
dgrogan should really review this Also will need histograms.xml owner review https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:61885: -<histogram_suffixes name="IndexedDBLevelDBPFEMethods" separator="."> Isn't this still needed to review older data? Or is it safe to remove? https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:12290: + Deprecated 2015-01. As of M41 use LevelDBEnv.IDB.BFE.NewLogger. Update all of these to 2015-03 / M42
https://codereview.chromium.org/862723002/diff/40001/third_party/leveldatabas... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/862723002/diff/40001/third_party/leveldatabas... third_party/leveldatabase/env_chromium.cc:964: base::HistogramBase* ChromiumEnv::GetMethodIOErrorHistogram() const { It looks like this one didn't need to change. It just records the Env methods where IOErrors were encountered, it doesn't record the IOErrors themselves. Edit (after looking at xml): Changing this like it is here brings us into a world of histograms.xml pain. I think you should revert the change in this method and leave its histograms named "LevelDBEnv.IOError" and "LevelDBEnv.IDB.IOError". As it stands now the data this CL logs to the new "LevelDBEnv.IDB.IOError.BFE.<methodName>" histograms would show up in the UMA UI with LevelDBIOErrorMethods labels, not PlatformFileError labels. https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:12272: +<histogram name="LevelDBEnv.IDB.IOError.BFE" enum="LevelDBIOErrorMethods"> Follow on to my comment from env_chromium.cc: remove this new <histogram> block. https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:12280: +<histogram name="LevelDBEnv.IDB.IOError.BFE." enum="PlatformFileError"> Remove the period from the end of the name attribute so that this will be the histogram that the new histogram_suffixes block below adds suffixes to. https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:12290: + Deprecated 2015-01. As of M41 use LevelDBEnv.IDB.BFE.NewLogger. On 2015/03/02 18:37:04, jsbell wrote: > Update all of these to 2015-03 / M42 Or whatever it ends up being. https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:12395: + Deprecated 2015-01. As of M41 use LevelDBEnv.IOError.BFE. Remove this <obsolete> block. https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:12410: + <summary>Methods where leveldb's Chromium environment has IO errors.</summary> Remove this new <histogram> block. https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:12413: +<histogram name="LevelDBEnv.IOError.BFE." enum="PlatformFileError"> And remove the period from the name attribute here too. https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:62128: + <affected-histogram name="WebCore.IndexedDB.LevelDBOpenErrors.PFE"/> These 3 WebCore entries should be BFE instead of PFE to review data collected in the new histograms created by this CL. To Josh's question above, I think we _also_ need the PFE entries here so that older data can be reviewed.
/git_cl.py crashed during the upload, but this patch looks intact. https://codereview.chromium.org/862723002/diff/40001/third_party/leveldatabas... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/862723002/diff/40001/third_party/leveldatabas... third_party/leveldatabase/env_chromium.cc:964: base::HistogramBase* ChromiumEnv::GetMethodIOErrorHistogram() const { On 2015/03/14 at 05:57:37, dgrogan wrote: > It looks like this one didn't need to change. It just records the Env methods where IOErrors were encountered, it doesn't record the IOErrors themselves. > > Edit (after looking at xml): > > Changing this like it is here brings us into a world of histograms.xml pain. I think you should revert the change in this method and leave its histograms named "LevelDBEnv.IOError" and "LevelDBEnv.IDB.IOError". > > As it stands now the data this CL logs to the new "LevelDBEnv.IDB.IOError.BFE.<methodName>" histograms would show up in the UMA UI with LevelDBIOErrorMethods labels, not PlatformFileError labels. Yes, I see your point - and agree. https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:61885: -<histogram_suffixes name="IndexedDBLevelDBPFEMethods" separator="."> On 2015/03/02 at 18:37:04, jsbell wrote: > Isn't this still needed to review older data? Or is it safe to remove? Yeah, I purposely combined this with LevelDBBFEMethods, but I'm not 100% that's the right thing to do, and even so is confusing. Putting back in. https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:12272: +<histogram name="LevelDBEnv.IDB.IOError.BFE" enum="LevelDBIOErrorMethods"> On 2015/03/14 at 05:57:37, dgrogan wrote: > Follow on to my comment from env_chromium.cc: remove this new <histogram> block. Done. https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:12280: +<histogram name="LevelDBEnv.IDB.IOError.BFE." enum="PlatformFileError"> On 2015/03/14 at 05:57:37, dgrogan wrote: > Remove the period from the end of the name attribute so that this will be the histogram that the new histogram_suffixes block below adds suffixes to. Done. https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:12290: + Deprecated 2015-01. As of M41 use LevelDBEnv.IDB.BFE.NewLogger. On 2015/03/14 at 05:57:37, dgrogan wrote: > On 2015/03/02 18:37:04, jsbell wrote: > > Update all of these to 2015-03 / M42 > > Or whatever it ends up being. Done. https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:12395: + Deprecated 2015-01. As of M41 use LevelDBEnv.IOError.BFE. On 2015/03/14 at 05:57:37, dgrogan wrote: > Remove this <obsolete> block. Done. https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:12410: + <summary>Methods where leveldb's Chromium environment has IO errors.</summary> On 2015/03/14 at 05:57:37, dgrogan wrote: > Remove this new <histogram> block. Done. https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:12413: +<histogram name="LevelDBEnv.IOError.BFE." enum="PlatformFileError"> On 2015/03/14 at 05:57:37, dgrogan wrote: > And remove the period from the name attribute here too. Done. https://codereview.chromium.org/862723002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:62128: + <affected-histogram name="WebCore.IndexedDB.LevelDBOpenErrors.PFE"/> On 2015/03/14 at 05:57:37, dgrogan wrote: > These 3 WebCore entries should be BFE instead of PFE to review data collected in the new histograms created by this CL. > > To Josh's question above, I think we _also_ need the PFE entries here so that older data can be reviewed. I restored the old IndexedDBLevelDBPFEMethods suffix table so no need for the PFE's here I think. Also, changing these PFE's to BFE's required adding WebCore.IndexedDB.LevelDBReadErrors.BFE and WebCore.IndexedDB.LevelDBWriteErrors.BFE, and obsoleting WebCore.IndexedDB.LevelDBReadErrors.PFE.
lgtm
The CQ bit was checked by cmumford@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/862723002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
cmumford@chromium.org changed reviewers: + mpearson@chromium.org - jsbell@chromium.org
+mpearson, Sorry: forgot to add an owner for histograms.xml
https://codereview.chromium.org/862723002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/862723002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:12558: + Deprecated 2015-05. As of M43 use LevelDBEnv.IDB.BFE.NewLogger. This histogram does not exist and is not added in this changelist. Perhaps you meant LevelDBEnv.IDB.IOError.BFE.NewLogger? ditto with 3 other obsolete messages below https://codereview.chromium.org/862723002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:12691: + Deprecated 2015-05. As of M43 use LevelDBEnv.IOError.BFE. Did you forget to append NewSequentialFile here? analogous comment with the two other obsolete message below
Thx Mark. https://codereview.chromium.org/862723002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/862723002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:12558: + Deprecated 2015-05. As of M43 use LevelDBEnv.IDB.BFE.NewLogger. On 2015/03/19 20:12:59, Mark P wrote: > This histogram does not exist and is not added in this changelist. > > Perhaps you meant LevelDBEnv.IDB.IOError.BFE.NewLogger? > > ditto with 3 other obsolete messages below Done. https://codereview.chromium.org/862723002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:12691: + Deprecated 2015-05. As of M43 use LevelDBEnv.IOError.BFE. On 2015/03/19 20:12:59, Mark P wrote: > Did you forget to append NewSequentialFile here? > > analogous comment with the two other obsolete message below Done - thx for catching that.
The CQ bit was checked by cmumford@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgrogan@chromium.org Link to the patchset: https://codereview.chromium.org/862723002/#ps80001 (title: "Fixed several obsolete messages to refer to correct histograms.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/862723002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
histograms.xml lgtm
The CQ bit was checked by mpearson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/862723002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by cmumford@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgrogan@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/862723002/#ps100001 (title: "Rebased on ToT (to very pretty histograms.xml) - no changes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/862723002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/58023982f5703b46008206f83f534d45dfd481de Cr-Commit-Position: refs/heads/master@{#322054} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
