New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 643573 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Sep 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocking:
issue 640921



Sign in to add a comment

Histograms matching Net.BidirectionalStream.* are unmapped

Project Member Reported by ricea@chromium.org, Sep 2 2016

Issue description

The following histograms are present in Chromium source code but do not
appear in histograms.xml.

Net.BidirectionalStream.HTTP2.ReceivedBytes defined at
net/http/bidirectional_stream.cc line 402
https://cs.chromium.org/chromium/src/net/http/bidirectional_stream.cc?l=402

Net.BidirectionalStream.HTTP2.SentBytes defined at
net/http/bidirectional_stream.cc line 404
https://cs.chromium.org/chromium/src/net/http/bidirectional_stream.cc?l=404

Net.BidirectionalStream.HTTP2.TimeToReadEnd defined at
net/http/bidirectional_stream.cc line 396
https://cs.chromium.org/chromium/src/net/http/bidirectional_stream.cc?l=396

Net.BidirectionalStream.HTTP2.TimeToReadStart defined at
net/http/bidirectional_stream.cc line 394
https://cs.chromium.org/chromium/src/net/http/bidirectional_stream.cc?l=394

Net.BidirectionalStream.HTTP2.TimeToSendEnd defined at
net/http/bidirectional_stream.cc line 400
https://cs.chromium.org/chromium/src/net/http/bidirectional_stream.cc?l=400

Net.BidirectionalStream.HTTP2.TimeToSendStart defined at
net/http/bidirectional_stream.cc line 398
https://cs.chromium.org/chromium/src/net/http/bidirectional_stream.cc?l=398

Net.BidirectionalStream.QUIC.ReceivedBytes defined at
net/http/bidirectional_stream.cc line 415
https://cs.chromium.org/chromium/src/net/http/bidirectional_stream.cc?l=415

Net.BidirectionalStream.QUIC.SentBytes defined at
net/http/bidirectional_stream.cc line 417
https://cs.chromium.org/chromium/src/net/http/bidirectional_stream.cc?l=417

Net.BidirectionalStream.QUIC.TimeToReadEnd defined at
net/http/bidirectional_stream.cc line 409
https://cs.chromium.org/chromium/src/net/http/bidirectional_stream.cc?l=409

Net.BidirectionalStream.QUIC.TimeToReadStart defined at
net/http/bidirectional_stream.cc line 407
https://cs.chromium.org/chromium/src/net/http/bidirectional_stream.cc?l=407

Net.BidirectionalStream.QUIC.TimeToSendEnd defined at
net/http/bidirectional_stream.cc line 413
https://cs.chromium.org/chromium/src/net/http/bidirectional_stream.cc?l=413

Net.BidirectionalStream.QUIC.TimeToSendStart defined at
net/http/bidirectional_stream.cc line 411
https://cs.chromium.org/chromium/src/net/http/bidirectional_stream.cc?l=411

File and line information may be out-of-date by the time you read this.

Please remove these histograms from the source code. If they were very
recently added, it may be worth adding them to histograms.xml instead, but
probably not.

This bug was automatically assigned based on git blame information. If you
are not the correct assignee for this bug, please delete the histograms
anyway.

 
Cc: isherman@chromium.org
The corresponding histograms are added to histograms.xml in https://codereview.chromium.org/2222113003.

I used "histogram_suffixes." Maybe that's why they were not found?

I don't want to remove them. Please let me know how I can fix the warning that you see.


I think those suffixes do not match the logged histograms, because of how ordering="prefix" works:

"When 'ordering="prefix"' is present in the histogram_suffixes tag, the suffix
will be inserted after the first dot separator of the affected-histogram name.
Therefore, the affected-histogram name has to have at least one dot in it"

"After the first dot" will be after "Net.", whereas you want it to be "before the last dot". 
Probably solution is to add support for another ordering="<something>" option that matches the "before last dot" semantics, and use that. Do you want to take a look at doing that?
Status: Started (was: Assigned)
Thanks a lot for the explanation. I will adjust the histograms. I think it's fine to have the suffix at the end in this case. It doesn't have to be before last dot.
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 2 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eef1d9c826ac10c339518a86d47dfdcf8da7cec0

commit eef1d9c826ac10c339518a86d47dfdcf8da7cec0
Author: xunjieli <xunjieli@chromium.org>
Date: Fri Sep 02 22:00:58 2016

Fix UMA logging for net::BidirectionalStream

The CL fixes the UMA to use suffixes correctly.
See  crbug.com/643573  for more info.

BUG= 635548 , 643573 

Review-Url: https://codereview.chromium.org/2311433002
Cr-Commit-Position: refs/heads/master@{#416359}

[modify] https://crrev.com/eef1d9c826ac10c339518a86d47dfdcf8da7cec0/net/http/bidirectional_stream.cc
[modify] https://crrev.com/eef1d9c826ac10c339518a86d47dfdcf8da7cec0/tools/metrics/histograms/histograms.xml

Ah, my bad -- I somehow misremembered how ordering="prefix" works when there are multiple dots.  Sorry for the incorrect advice previously!
Status: Fixed (was: Started)
Thanks everyone!
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 6 2016

Labels: merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a731b8734b0b46fb7114f4064d0f777243b41eb9

commit a731b8734b0b46fb7114f4064d0f777243b41eb9
Author: xunjieli <xunjieli@chromium.org>
Date: Tue Sep 06 14:56:46 2016

Fix UMA logging for net::BidirectionalStream

The CL fixes the UMA to use suffixes correctly.
See  crbug.com/643573  for more info.

BUG= 635548 , 643573 

NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2311433002
Cr-Commit-Position: refs/heads/master@{#416359}
(cherry picked from commit eef1d9c826ac10c339518a86d47dfdcf8da7cec0)

Review-Url: https://codereview.chromium.org/2311793003
Cr-Commit-Position: refs/branch-heads/2840@{#169}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/a731b8734b0b46fb7114f4064d0f777243b41eb9/net/http/bidirectional_stream.cc
[modify] https://crrev.com/a731b8734b0b46fb7114f4064d0f777243b41eb9/tools/metrics/histograms/histograms.xml

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a731b8734b0b46fb7114f4064d0f777243b41eb9

commit a731b8734b0b46fb7114f4064d0f777243b41eb9
Author: xunjieli <xunjieli@chromium.org>
Date: Tue Sep 06 14:56:46 2016

Fix UMA logging for net::BidirectionalStream

The CL fixes the UMA to use suffixes correctly.
See  crbug.com/643573  for more info.

BUG= 635548 , 643573 

NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2311433002
Cr-Commit-Position: refs/heads/master@{#416359}
(cherry picked from commit eef1d9c826ac10c339518a86d47dfdcf8da7cec0)

Review-Url: https://codereview.chromium.org/2311793003
Cr-Commit-Position: refs/branch-heads/2840@{#169}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/a731b8734b0b46fb7114f4064d0f777243b41eb9/net/http/bidirectional_stream.cc
[modify] https://crrev.com/a731b8734b0b46fb7114f4064d0f777243b41eb9/tools/metrics/histograms/histograms.xml

Sign in to add a comment