New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

I added more decimal digits to the timing statistics counters #1106

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@maurorappa

maurorappa commented Nov 4, 2016

in order to provide more detailed performance statistics, I changed the printf to use 6 decimals.
Luckily enough the metrics were already using double integers.

maurorappa
I added more decimal digits to the timing statistics counters, in ord…
…er to provide more detailed performance statistics
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Nov 4, 2016

@maurorappa, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @gevaerts and @yangtse to be potential reviewers.

mention-bot commented Nov 4, 2016

@maurorappa, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @gevaerts and @yangtse to be potential reviewers.

@bagder bagder added the cmdline tool label Nov 5, 2016

@bagder bagder closed this in ebeffe8 Nov 5, 2016

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 5, 2016

Member

thanks!

Member

bagder commented Nov 5, 2016

thanks!

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Nov 5, 2016

Member

Could this conceivably break some scripts if they expect data in a 0.000 and is microsecond really necessary for those statistics? All I get is a lot of x.xxx000

Member

jay commented Nov 5, 2016

Could this conceivably break some scripts if they expect data in a 0.000 and is microsecond really necessary for those statistics? All I get is a lot of x.xxx000

@maurorappa

This comment has been minimized.

Show comment
Hide comment
@maurorappa

maurorappa Nov 5, 2016

This is my output on Ubuntu Trusty:

maurorappa@trusty:~/curl/src$ ./curl -sLk -w "%{http_code}\n%{time_total}: %{time_namelookup} %{time_connect} %{time_appconnect}\n%{speed_download} %{num_redirects}\n" https://dcs.ida.digital.cabinet-office.gov.uk -o /dev/null
400
0.083220: 0.067998 0.069151 0.082273
2956.000 0

I can do statistical analysis of the SSL performance with more granularity.

maurorappa commented Nov 5, 2016

This is my output on Ubuntu Trusty:

maurorappa@trusty:~/curl/src$ ./curl -sLk -w "%{http_code}\n%{time_total}: %{time_namelookup} %{time_connect} %{time_appconnect}\n%{speed_download} %{num_redirects}\n" https://dcs.ida.digital.cabinet-office.gov.uk -o /dev/null
400
0.083220: 0.067998 0.069151 0.082273
2956.000 0

I can do statistical analysis of the SSL performance with more granularity.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Nov 5, 2016

Member

Ok. time_total is documented as millisecond resolution so that's something we have to consider. And I suspect it becomes somewhat more arbitrary after that since it's basically wall clock time. What if there was a variable like %{flags:microseconds,foo,bar}

Member

jay commented Nov 5, 2016

Ok. time_total is documented as millisecond resolution so that's something we have to consider. And I suspect it becomes somewhat more arbitrary after that since it's basically wall clock time. What if there was a variable like %{flags:microseconds,foo,bar}

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 5, 2016

Member

Ah yes good catch, time_total does in fact have that mentioned in the docs. I consider that a bug though and no other time variable has that detail mentioned.

Member

bagder commented Nov 5, 2016

Ah yes good catch, time_total does in fact have that mentioned in the docs. I consider that a bug though and no other time variable has that detail mentioned.

@maurorappa

This comment has been minimized.

Show comment
Hide comment
@maurorappa

maurorappa Nov 5, 2016

I can fix the man page if it helps.

maurorappa commented Nov 5, 2016

I can fix the man page if it helps.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 5, 2016

Member

Yes please, the millisecond mention is wrong now since commit ebeffe8

Member

bagder commented Nov 5, 2016

Yes please, the millisecond mention is wrong now since commit ebeffe8

jay added a commit to jay/curl that referenced this pull request Nov 8, 2016

tool_writeout: add %{flags:} param. draft1
- Add %{flags:<flag>,...} to --write-out

- New write-out flag stderr to redirect to stderr

- New write-out flag moreprec to increase precision from 3 to 6

---

This is an alternative proposal to adding more precision by default.
curl#1106
@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Nov 8, 2016

Member

I think we should not do this by default, and that adding it as an option would be better. To that end I've written a counterproposal, you can see it at https://github.com/curl/curl/compare/master...jay:add_flags_to_write-out?expand=1

curld -sS --write-out " %{time_total} %{flags: moreprec,stderr} %{time_total}" example.com -o NUL 2>NUL
 0.047

curld -sS --write-out " %{time_total} %{flags: moreprec,stderr} %{time_total}" example.com -o NUL 1>NUL
 0.047000
Member

jay commented Nov 8, 2016

I think we should not do this by default, and that adding it as an option would be better. To that end I've written a counterproposal, you can see it at https://github.com/curl/curl/compare/master...jay:add_flags_to_write-out?expand=1

curld -sS --write-out " %{time_total} %{flags: moreprec,stderr} %{time_total}" example.com -o NUL 2>NUL
 0.047

curld -sS --write-out " %{time_total} %{flags: moreprec,stderr} %{time_total}" example.com -o NUL 1>NUL
 0.047000
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 8, 2016

Member

I think we should not do this by default

Because of the risk that it will break some scripts? Because some machines won't have better accuracy than milliseconds anyway?

What about inventing some additional syntax hint that allows users to specify precision per variable. like %{time_total@6} and %{time_total@3} ?

Member

bagder commented Nov 8, 2016

I think we should not do this by default

Because of the risk that it will break some scripts? Because some machines won't have better accuracy than milliseconds anyway?

What about inventing some additional syntax hint that allows users to specify precision per variable. like %{time_total@6} and %{time_total@3} ?

@bagder bagder reopened this Nov 8, 2016

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Nov 8, 2016

Member

For all those reasons, but also with respect to the reporter measurements based off of differences in wall clock time are only so accurate. Your way is interesting, however I like the idea of %{flags regardless because it's extensible

Member

jay commented Nov 8, 2016

For all those reasons, but also with respect to the reporter measurements based off of differences in wall clock time are only so accurate. Your way is interesting, however I like the idea of %{flags regardless because it's extensible

jay added a commit to jay/curl that referenced this pull request Nov 28, 2016

tool_writeout: add %{flags:} param. draft2
- Add %{flags:<flag>,...} to --write-out

- New write-out flag 'fd[1-9]|stderr|stdout' to write to a specific fd

- New write-out flag 'prec[0-99]' to increase precision of floats

---

This is an alternative proposal to adding more precision by default.
curl#1106
@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Nov 28, 2016

Member

I just finished draft2 of this, if the feature window hasn't closed yet and there is interest. I still need a test.

flags Flags to alter behavior. (Added in 7.52.0)

'prec[0-99]' Use this precision in floating point values instead of 3. Note if
your OS doesn't support the higher resolution then all zeroes may be output for
the extra digits, eg 0.047000 (Added in 7.52.0)

'fd[1-9]|stderr|stdout' Write the output to a file descriptor instead of the
default stdout. Note stderr isn't recommended since it is used for errors and
there's a possiblity an error may disrupt the write-out text. Also this is not
entirely portable; whether it works depends on your shell and build of curl.
(Added in 7.52.0)

Each flag can be used multiple times and at any point in the write out text
unless otherwise stated. To use the flags variable follow it with a colon and
zero or more flags separated by a comma, optional whitespace (ignored) and
optionally prefixed by a modifier. An unprefixed or + prefixed flag is turn on
and a - prefixed flag is turn off. For example %{flags: stderr, -prec6}.

Conceivably in Linux and cygwin it should work on 3>foo and if %{flags:fd3} the write-out text will go there. It won't work in Windows.

Member

jay commented Nov 28, 2016

I just finished draft2 of this, if the feature window hasn't closed yet and there is interest. I still need a test.

flags Flags to alter behavior. (Added in 7.52.0)

'prec[0-99]' Use this precision in floating point values instead of 3. Note if
your OS doesn't support the higher resolution then all zeroes may be output for
the extra digits, eg 0.047000 (Added in 7.52.0)

'fd[1-9]|stderr|stdout' Write the output to a file descriptor instead of the
default stdout. Note stderr isn't recommended since it is used for errors and
there's a possiblity an error may disrupt the write-out text. Also this is not
entirely portable; whether it works depends on your shell and build of curl.
(Added in 7.52.0)

Each flag can be used multiple times and at any point in the write out text
unless otherwise stated. To use the flags variable follow it with a colon and
zero or more flags separated by a comma, optional whitespace (ignored) and
optionally prefixed by a modifier. An unprefixed or + prefixed flag is turn on
and a - prefixed flag is turn off. For example %{flags: stderr, -prec6}.

Conceivably in Linux and cygwin it should work on 3>foo and if %{flags:fd3} the write-out text will go there. It won't work in Windows.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jul 30, 2017

Member

I think we can just leave the digits as they are as nobody has reported any problems with this change so far.

Member

bagder commented Jul 30, 2017

I think we can just leave the digits as they are as nobody has reported any problems with this change so far.

@stale

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot Oct 27, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale bot commented Oct 27, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 27, 2017

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Oct 27, 2017

Member

I think we can just leave the digits as they are as nobody has reported any problems with this change so far.

Ok

Member

jay commented Oct 27, 2017

I think we can just leave the digits as they are as nobody has reported any problems with this change so far.

Ok

@jay jay closed this Oct 27, 2017

@jay jay added stale and removed stale labels Oct 27, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 13, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.