Supports to strip leading zero on print_log2_hist function #1226

thnam91 · 2017-06-20T15:35:41Z

I found #1109 issue about print_log2_hist function and read its comments.

I implemented a option that doesn't show the leading zeros.

I added argument strip_leading_zero to print_log2_hist function and its default value is None.

And I applied it to funclatency.py, funclatency_example.txt, biolatency.py, and biolatency_example.txt.

Instead of this:

# ./funclatency.py do_sys_open
Tracing 1 functions for "do_sys_open"... Hit Ctrl-C to end.
^C
     nsecs               : count     distribution
         0 -> 1          : 0        |                                        |
         2 -> 3          : 0        |                                        |
         4 -> 7          : 0        |                                        |
         8 -> 15         : 0        |                                        |
        16 -> 31         : 0        |                                        |
        32 -> 63         : 0        |                                        |
        64 -> 127        : 0        |                                        |
       128 -> 255        : 0        |                                        |
       256 -> 511        : 0        |                                        |
       512 -> 1023       : 0        |                                        |
      1024 -> 2047       : 189      |**                                      |
      2048 -> 4095       : 2646     |****************************************|
      4096 -> 8191       : 2563     |**************************************  |
      8192 -> 16383      : 2107     |*******************************         |
     16384 -> 32767      : 302      |****                                    |
     32768 -> 65535      : 79       |*                                       |
     65536 -> 131071     : 14       |                                        |
Detaching...

Print this using -s option:

# ./funclatency.py -s do_sys_open
Tracing 1 functions for "do_sys_open"... Hit Ctrl-C to end.
^C
     nsecs               : count     distribution
       512 -> 1023       : 9        |                                        |
      1024 -> 2047       : 925      |*******                                 |
      2048 -> 4095       : 4781     |****************************************|
      4096 -> 8191       : 3532     |*****************************           |
      8192 -> 16383      : 625      |*****                                   |
     16384 -> 32767      : 163      |*                                       |
     32768 -> 65535      : 26       |                                        |
     65536 -> 131071     : 3        |                                        |
    131072 -> 262143     : 0        |                                        |
    262144 -> 524287     : 0        |                                        |
    524288 -> 1048575    : 0        |                                        |
   1048576 -> 2097151    : 0        |                                        |
   2097152 -> 4194303    : 0        |                                        |
   4194304 -> 8388607    : 0        |                                        |
   8388608 -> 16777215   : 0        |                                        |
  16777216 -> 33554431   : 0        |                                        |
  33554432 -> 67108863   : 1        |                                        |
Detaching...

And Instead of this:

# ./biolatency.py 
...
Tracing block device I/O... Hit Ctrl-C to end.
^C
     usecs               : count     distribution
         0 -> 1          : 0        |                                        |
         2 -> 3          : 0        |                                        |
         4 -> 7          : 0        |                                        |
         8 -> 15         : 0        |                                        |
        16 -> 31         : 0        |                                        |
        32 -> 63         : 5        |****                                    |
        64 -> 127        : 6        |*****                                   |
       128 -> 255        : 2        |*                                       |
       256 -> 511        : 0        |                                        |
       512 -> 1023       : 0        |                                        |
      1024 -> 2047       : 0        |                                        |
      2048 -> 4095       : 41       |****************************************|
      4096 -> 8191       : 2        |*                                       |

Print this using -s option:

# ./biolatency.py -s
...
Tracing block device I/O... Hit Ctrl-C to end.
^C
     usecs               : count     distribution
        32 -> 63         : 2        |***********                             |
        64 -> 127        : 1        |*****                                   |
       128 -> 255        : 1        |*****                                   |
       256 -> 511        : 1        |*****                                   |
       512 -> 1023       : 0        |                                        |
      1024 -> 2047       : 0        |                                        |
      2048 -> 4095       : 7        |****************************************|

Please review my code and comments about it. :D

If it can cause any other problems, please let me know how can I fix it.

Thank you.

goldshtn · 2017-06-22T02:18:56Z

[buildbot, ok to test]

goldshtn · 2017-06-22T02:20:30Z

tools/biolatency.py

@@ -36,6 +37,8 @@
 help="millisecond histogram")
 parser.add_argument("-D", "--disks", action="store_true",
 help="print a histogram per disk device")
+parser.add_argument("-s", "--strip", action="store_true",
+ help="strip leading zero on output")


I don't think this makes a lot of sense as an additional option for any tool. This is a decision the tool author should make. And in fact, I don't see why we wouldn't want this behavior by default -- so probably in most cases, the choice should be to strip leading zeros. @brendangregg

brendangregg · 2017-06-22T05:08:06Z

Yes, I think it should be up to the tool author. Some tools may want this as the default, some not. Adding an -s option should be an exception, only if really needed, and not on everything as it would clutter up their usage.

This was discussed briefly once before on #1109, as I'd suggested dropping the zeros by default, but someone pointed out that it can be a good visual clue.

Maybe... if the metric is latency, the zeros could be kept for that visual clue: as higher is bad. But for other metrics, like size, I'm not sure printing zeros matter. Hence, again, the tool author can pick the default that makes sense.

thnam91 · 2017-06-22T07:03:36Z

I really agree that the default value is up to the tool author. Keeping zeros is currently the default option on both funclatency.py and biolatency.py because I also agree @frisso's opinion in #1109.

Actually, I'm not a initial tool author of funclatency.py and biolatency.py. So I'm very careful to change tools because I don't want to spoil the intention of the original author for funclatency.py and biolatency.py tools.

Is there anything I need to correct or impove before -s option is merged?

goldshtn · 2017-06-22T10:59:26Z

Yes - I think Brendan (the author of these tools) and I are saying that we don't need the -s switch.

thnam91 · 2017-06-22T13:28:58Z

Thanks to reply. Because I'm not good at English, I'm confused something. As you and Brendan discussed the default value, isn't it necessary to add -s option for only latency metric? And the metric both funclatency.py and biolatency.py is the latency. So I understood that -s switch is necessary, isn't it right?

If we don't need -s option anymore, do I close this pull request?

goldshtn · 2017-06-22T13:39:37Z

We are saying that the tool should decide whether to use the new stripping behavior or not. It should not be exposed to the user as a -s switch. So you can make funclatency and biolatency not strip the leading zeros, per Brendan's suggestion, but you don't need the -s switch for that.

Also, why close the PR? You can push a new commit that makes the changes you want and the PR will be updated automatically.

thnam91 · 2017-06-22T14:55:13Z

Thanks to reply again. It's hard to understand contribution processes because I'm not familiar with code contribution. But I'm being able to get a great experience in this time. Also, I'm learning how to communicate with contributors and how can my code be applied on open source project.

As a result, funclatency.py and biolatency.py don't need -s switch and strip leading zeros, is it right?

I'm really sorry to bother you, but I'm curious about something. We already decided that strip leading zero function is not necessary. So #1109 issue is solved as our decision. But this PR is for #1109. Therefore if I push a new commit about another issue, I should make new PR. Isn't it?

goldshtn · 2017-06-23T08:10:41Z

What makes this PR "about #1109"? You can edit the description and the commits to whatever you want. I think at this point we should make funclatency and biolatency strip zeros, and have the library function as is. No need for -s.

thnam91 · 2017-06-23T15:51:00Z

Due to your advices, I was able to understand what you and he said. And I realized that the tool author should decide to strip leading zeros or not.

So I removed the python code and fixed the description for -s switch in funclatency and biolatency. Also, I only added an argument, 'strip_leading_zero', in the print_log2_hist function and its helper function in table.py.

If author use print_log2_hist function using new argument like strip_leading_zero=1, print a histogram without leading zeros.
If author use print_log2_hist function without strip_leading_zero argument, print a histogram including consecutive zeros from beginning.

I really hope that I understood properly what you mean. Could you give me any other advice or comment?

goldshtn · 2017-06-28T06:24:24Z

I think at this point we should make funclatency and biolatency strip zeros, and have the library function as is

Your latest patchset doesn't do this. You added the strip_leading_zero option to the Python API, but currently none of the scripts use it. We agreed that funclatency and biolatency should pass True to strip leading zeros.

thnam91 · 2017-06-28T06:49:27Z

Because of my English skill, I completely misunderstood something. I'm so sorry.
Finally, I updated funclatency and biolatency tools that should pass True to strip leading zeros.

I think that Eng comprehension is harder than code comprehension T_T.....
Anyway, thank you so much to reply.

goldshtn · 2017-06-28T12:01:32Z

@thnam91 It's not your English, it's actually me who misunderstood Brendan's original comment. That's what happens when you triage pull requests while jetlagged. What Brendan was saying is that the latency-related tools should not strip the leading zeros from the histogram.

So the library change (in table.py) is fine, but we should revert to the version that doesn't change funclatency and biolatency -- they should not strip zeros.

Sorry for all the back and forth, it really is my fault.

thnam91 · 2017-06-28T12:19:59Z

Thank you so much :D. Finally I understood this issue clearly. I hope that this PR will be my final pull request.

goldshtn · 2017-06-29T06:01:21Z

tools/biolatency.py

@@ -131,7 +132,7 @@
 if args.timestamp:
 print("%-8s\n" % strftime("%H:%M:%S"), end="")

- dist.print_log2_hist(label, "disk")
+ dist.print_log2_hist(label, "disk", strip_leading_zero=None)


I don't think this is needed because None is the default value.

goldshtn · 2017-06-29T06:01:30Z

tools/funclatency.py

@@ -237,9 +238,9 @@ def print_section(key):

 if need_key:
 dist.print_log2_hist(label, "Function", section_print_fn=print_section,
- bucket_fn=lambda k: (k.ip, k.pid))
+ bucket_fn=lambda k: (k.ip, k.pid), strip_leading_zero=None)


Same here and below.

goldshtn · 2017-06-29T06:09:25Z

tools/biolatency.py

@@ -10,6 +10,7 @@
 # Licensed under the Apache License, Version 2.0 (the "License")
 #
 # 20-Sep-2015 Brendan Gregg Created this.
+# 28-Jun-2017 Taekho Nam Not stripped leading zero.


I'm sorry, but there are no changes in biolatency and funclatency any more, so these comments also seem unnecessary.

thnam91 · 2017-06-29T08:20:59Z

You are right. It is not unnecessary any more. Thank you. I missed it.

goldshtn · 2017-06-29T14:20:57Z

Thanks, merged.

Taekho Nam added 2 commits June 21, 2017 00:32

Add srtip_leading_zero to print_log2_hist function

fe2f15e

Add a '-s' option to strip leading zeros

362c6db

goldshtn reviewed Jun 22, 2017

View reviewed changes

Taekho Nam added 3 commits June 23, 2017 19:15

Delete -s switch from tools named funclatency.py and biolatency.py

a43e89b

Add new argument for print_log2_hist function for stripping left zeros

582b496

Fix a typo in the docstring for print_log2_hist function

30b64f8

Stripped leading zeros on latency metric

2f063fa

Add some comments for author

2abe9ea

Revert to the version that does not strip leading zeros

bd1b55b

goldshtn reviewed Jun 29, 2017

View reviewed changes

Delete strip_leading_zero=None, None is the default value

b66fb74

goldshtn reviewed Jun 29, 2017

View reviewed changes

Delete unnecessary author information

727a6f5

goldshtn merged commit 0bd1a6d into iovisor:master Jun 29, 2017

thnam91 deleted the issue-solve branch June 29, 2017 14:42

tchajed mentioned this pull request May 13, 2021

print_log2_hist to strip leading zeros #1109

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Supports to strip leading zero on print_log2_hist function #1226

Supports to strip leading zero on print_log2_hist function #1226

thnam91 commented Jun 20, 2017

goldshtn commented Jun 22, 2017

goldshtn Jun 22, 2017

brendangregg commented Jun 22, 2017

thnam91 commented Jun 22, 2017 •

edited

Loading

goldshtn commented Jun 22, 2017

thnam91 commented Jun 22, 2017

goldshtn commented Jun 22, 2017

thnam91 commented Jun 22, 2017

goldshtn commented Jun 23, 2017

thnam91 commented Jun 23, 2017

goldshtn commented Jun 28, 2017

thnam91 commented Jun 28, 2017

goldshtn commented Jun 28, 2017

thnam91 commented Jun 28, 2017

goldshtn Jun 29, 2017

goldshtn Jun 29, 2017

goldshtn Jun 29, 2017

thnam91 commented Jun 29, 2017

goldshtn commented Jun 29, 2017

Supports to strip leading zero on print_log2_hist function #1226

Supports to strip leading zero on print_log2_hist function #1226

Conversation

thnam91 commented Jun 20, 2017

goldshtn commented Jun 22, 2017

goldshtn Jun 22, 2017

Choose a reason for hiding this comment

brendangregg commented Jun 22, 2017

thnam91 commented Jun 22, 2017 • edited Loading

goldshtn commented Jun 22, 2017

thnam91 commented Jun 22, 2017

goldshtn commented Jun 22, 2017

thnam91 commented Jun 22, 2017

goldshtn commented Jun 23, 2017

thnam91 commented Jun 23, 2017

goldshtn commented Jun 28, 2017

thnam91 commented Jun 28, 2017

goldshtn commented Jun 28, 2017

thnam91 commented Jun 28, 2017

goldshtn Jun 29, 2017

Choose a reason for hiding this comment

goldshtn Jun 29, 2017

Choose a reason for hiding this comment

goldshtn Jun 29, 2017

Choose a reason for hiding this comment

thnam91 commented Jun 29, 2017

goldshtn commented Jun 29, 2017

thnam91 commented Jun 22, 2017 •

edited

Loading