Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
327163003:
Update FTP server type histograms in histograms.xml (Closed)
|
Created:
6 years, 6 months ago by davidben Modified:
6 years, 6 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, asvitkine+watch_chromium.org, egrannum_google.com, rmorina_google.com Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionUpdate FTP server type histograms in histograms.xml
They were changed in https://chromiumcodereview.appspot.com/11360211, but
histograms.xml reflects the old one.
BUG=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278550
Patch Set 1 #
Total comments: 7
Patch Set 2 : asvitkine comments #Patch Set 3 : 2012, not 2011. #
Messages
Total messages: 11 (0 generated)
I'm unclear that the old enum definition actually matches the old version of the list in the first place, but I left it alone and made a copy in case anything cared. egrannum, rmorina: FYI.
Thanks, the clean up makes sense (though it would have been good to have this change be part of the same CL). I gave some suggestions on how the wording for these can be improved a little bit. https://codereview.chromium.org/327163003/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/327163003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:13480: + Replaced by Net.FtpServerTypeCount2. Nit: Specify a date when the changed landed. Same below. https://codereview.chromium.org/327163003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:13559: + Each bucket is a boolean (0 or 1) indicating whether the user has had a Commenting on this description, but comment applies to the new histogram entry, really. This description isn't quite right. A bucket isn't a boolean, rather it's a count of the number connections of that type that occurred. I'd also remove "during the session" wording, since that's confusing and not quite correct depending on one's definition of "session". https://codereview.chromium.org/327163003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:37673: + net/ftp/ftp_server_type_histograms.h Can these be marked obsolete too? I don't recall if enums can be marked obsolete, but I think they can?
Also, it's not super clear to me what's the difference between the two histograms. Naively, it seems like they might be measuring the same thing. Am I missing something? If so, can that be mentioned more clearly in their descriptions? On Thu, Jun 19, 2014 at 3:02 PM, <asvitkine@chromium.org> wrote: > Thanks, the clean up makes sense (though it would have been good to have > this > change be part of the same CL). > > I gave some suggestions on how the wording for these can be improved a > little > bit. > > > https://codereview.chromium.org/327163003/diff/1/tools/ > metrics/histograms/histograms.xml > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/327163003/diff/1/tools/ > metrics/histograms/histograms.xml#newcode13480 > tools/metrics/histograms/histograms.xml:13480: + Replaced by > Net.FtpServerTypeCount2. > Nit: Specify a date when the changed landed. Same below. > > https://codereview.chromium.org/327163003/diff/1/tools/ > metrics/histograms/histograms.xml#newcode13559 > tools/metrics/histograms/histograms.xml:13559: + Each bucket is a > > boolean (0 or 1) indicating whether the user has had a > Commenting on this description, but comment applies to the new histogram > entry, really. > > This description isn't quite right. A bucket isn't a boolean, rather > it's a count of the number connections of that type that occurred. I'd > also remove "during the session" wording, since that's confusing and not > quite correct depending on one's definition of "session". > > https://codereview.chromium.org/327163003/diff/1/tools/ > metrics/histograms/histograms.xml#newcode37673 > tools/metrics/histograms/histograms.xml:37673: + > net/ftp/ftp_server_type_histograms.h > Can these be marked obsolete too? I don't recall if enums can be marked > obsolete, but I think they can? > > https://codereview.chromium.org/327163003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/327163003/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/327163003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:13559: + Each bucket is a boolean (0 or 1) indicating whether the user has had a On 2014/06/19 19:02:38, Alexei Svitkine wrote: > Commenting on this description, but comment applies to the new histogram entry, > really. > > This description isn't quite right. A bucket isn't a boolean, rather it's a > count of the number connections of that type that occurred. I'd also remove > "during the session" wording, since that's confusing and not quite correct > depending on one's definition of "session". Nevermind, I see the distinction this is trying to capture. Maybe a more clear description that a given bucket's value is logged with a count of 1 the first time an FTP server of that type is accessed within a session.
On 2014/06/19 19:02:39, Alexei Svitkine wrote: > Thanks, the clean up makes sense (though it would have been good to have this > change be part of the same CL). Oh. Err, sorry that might have been unclear. The clean-up was over a year ago. I'd only just noticed that histograms.xml never got updated for it. :-)
https://codereview.chromium.org/327163003/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/327163003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:13480: + Replaced by Net.FtpServerTypeCount2. On 2014/06/19 19:02:38, Alexei Svitkine wrote: > Nit: Specify a date when the changed landed. Same below. Done. https://codereview.chromium.org/327163003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:13559: + Each bucket is a boolean (0 or 1) indicating whether the user has had a On 2014/06/19 19:07:46, Alexei Svitkine wrote: > On 2014/06/19 19:02:38, Alexei Svitkine wrote: > > Commenting on this description, but comment applies to the new histogram > entry, > > really. > > > > This description isn't quite right. A bucket isn't a boolean, rather it's a > > count of the number connections of that type that occurred. I'd also remove > > "during the session" wording, since that's confusing and not quite correct > > depending on one's definition of "session". > > Nevermind, I see the distinction this is trying to capture. Maybe a more clear > description that a given bucket's value is logged with a count of 1 the first > time an FTP server of that type is accessed within a session. Yeah, that wording is really weird. How are these new ones? https://codereview.chromium.org/327163003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:37673: + net/ftp/ftp_server_type_histograms.h On 2014/06/19 19:02:38, Alexei Svitkine wrote: > Can these be marked obsolete too? I don't recall if enums can be marked > obsolete, but I think they can? Looks like it. Done.
Yes, much better. Thanks! (And yes, I hadn't realised that the cleanup happened a year ago and was not something recent.)
lgtm
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/327163003/40001
Message was sent while issue was closed.
Change committed as 278550 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
