dump_float() poor output #29545

dzafman · 2019-08-07T23:54:33Z

Checklist

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

Show available Jenkins commands

jenkins retest this please
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test docs
jenkins render docs

This reverts commit 28253a5. Fixes: https://tracker.ceph.com/issues/41156 Signed-off-by: David Zafman <dzafman@redhat.com>

dzafman · 2019-08-07T23:55:46Z

This reverts #25745

tchaikov · 2019-08-08T14:35:18Z

src/common/Formatter.cc

@@ -293,7 +291,9 @@ void JSONFormatter::dump_int(const char *name, int64_t s)

 void JSONFormatter::dump_float(const char *name, double d)
 {
-  add_value(name, d);
+  char foo[30];
+  snprintf(foo, sizeof(foo), "%lf", d);


i think the reason we have 0.8100000000000001 when printing 810 / 1000.0 is just that IEEE754 is not able represent all decimal numbers with a fraction part. the default precision of printf() is 6. that's why we have 0.810000 if we print the number using printf() without specifying the number of digits. if we instead use

char foo[30]; double f = 810 / 1000.0; snprintf(foo, sizeof(foo), "%.*f", std::numeric_limits<decltype(f)>::max_digits10, f); cout << foo << endl;

we will have exactly the same output as

cout.precision(std::numeric_limits<double>::max_digits10); cout << f << endl;

but i understand your concern -- a nice looking decimal number is printed as if it has had very small fraction part. but the reality is that it does =(
the default precision of 6 just hides the ugly fact.

I'm not really sure what the issue is with high precision numbers in JSON output which is not meant to be human consumable. @dzafman is this really a problem?

@batrick @tchaikov I'm keeping everything in microseconds which could also be considered milliseconds in a fix point representation with scale 1000 (3 digits). So I'm thinking of replacing the N / 1000.0 with something that creates a string from an integer and the decimal scale as a number of digits. There's a little leading 0 stuff to handle too.

1234 -> 1.234
234 -> 0.234
2 -> 0.002

To me this is human consumable from the command line

ceph daemon mgr.x dump_osd_network

This corresponds to new ceph health output which is using default float stream precision:

ceph health

OSD_SLOW_PING_TIME_FRONT Long heartbeat ping times on front interface seen, longest is 1.372 msec
Slow heartbeat ping on front interface from osd.0 to osd.2 1.372 msec

ceph daemon mgr.x dump_osd_network

...
{
"last update": "Wed Aug 7 19:51:30 2019",
"stale": false,
"from osd": 0,
"to osd": 2,
"interface": "front",
"average": {
"1min": 1.242,
"5min": 1.3720000000000001,
"15min": 1.345
},
...

@batrick @tchaikov I'm going to avoid using dump_float(), so I don't care if we close this pull request. I think we should lower the precision by 1 (or 2) from the maximum so that rounding hides the issue with representation of decimal numbers.

Ceph utils should give plain format by default. JSON output should be printed only if requested explicitly. So, it's better to change default format for this command.

…ecision Signed-off-by: David Zafman <dzafman@redhat.com>

dzafman · 2019-08-24T00:34:59Z

Won't fix this

dzafman · 2019-08-30T22:47:35Z

@tchaikov @batrick Looking at a test run "ceph osd dump" output we see this ugly output:

"full_ratio": 0.949999988079071,
"backfillfull_ratio": 0.8999999761581421,
"nearfull_ratio": 0.8500000238418579,

  "weight": 0.904632568359375,
  "primary_affinity": 0.451995849609375,

Do we really want this which IS viewed by humans? The full ratios are .95, .90 .85 respectively. ~~Not sure why this comes out this way when it is in the configuration as a float with those values.~~ Do we really need that much precision for "weight" or "primary_affinity"?

dzafman · 2019-09-03T23:23:14Z

@tchaikov @batrick This user command outputs a JSON result. I don't think we need that many digits of precision.

$ ceph daemon osd.0 compact
{
"elapsed_time": 0.087996230999999994
}

The osd log output stream has plenty of digits (7) of precision after the decimal point.
2019-09-03T23:17:55.321+0000 7f89c738e700 1 osd.0 14 triggering manual compaction
2019-09-03T23:17:55.409+0000 7f89c738e700 1 osd.0 14 finished manual compaction in 0.0879962 seconds

tchaikov · 2019-09-07T01:25:24Z

@socketpair what do you think? @dzafman or, probably a compromise is to limit the precision of certain output?

socketpair · 2019-09-07T03:55:13Z

Json is not intended to humans. Use jq (for example) to colorize output and to control precision.

Plain output precision should be restricted, yes. And also numbers should be formatted using locale settings, since purpose is to read by humans.

It’s possible to add new option, but it would increase code base. I vote against.

socketpair · 2019-09-08T18:12:53Z

Just an example:

$ ceph osd stat
16 osds: 16 up (since 3d), 16 in (since 2w); epoch: e75323

$ ceph -f json-pretty osd stat
{
    "epoch": 75323,
    "num_osds": 16,
    "num_up_osds": 16,
    "num_in_osds": 16,
    "full": false,
    "nearfull": false,
    "num_remapped_pgs": 0
}

dzafman · 2019-09-09T15:41:29Z

@socketpair Humans see JSON output even if it isn't meant for them. There is no need for maximum precision which distorts results due to base10 output. By cutting back the precision we hide this binary to base10 inaccuracy.

socketpair · 2019-09-09T17:44:24Z

@dzafman no, if humans want to see JSON, they should use tools like jq. Well, for some fields it is desirable to limit precision. For others — definitely not (that’s why I made this change). Setting precision in every place or in some picky fields is not a good idea, as I think.

It’s better to add non-json (plain) output for commands you concern on.

dzafman · 2019-09-16T22:03:41Z

Closing this pull request. I suggest we change many dump_float() usage to dump_stream() for floating point output we don't need full precision. See https://tracker.ceph.com/issues/41754

Revert "Formatters: improve precision of double numbers"

48a1275

This reverts commit 28253a5. Fixes: https://tracker.ceph.com/issues/41156 Signed-off-by: David Zafman <dzafman@redhat.com>

dzafman requested review from tchaikov and batrick August 7, 2019 23:57

dzafman added common tests labels Aug 7, 2019

tchaikov reviewed Aug 8, 2019

View reviewed changes

test: Do a long float test which for now is defaulting to 6 digits pr…

3fa28eb

…ecision Signed-off-by: David Zafman <dzafman@redhat.com>

dzafman force-pushed the wip-41156 branch from 67343ea to 3fa28eb Compare August 8, 2019 15:19

dzafman closed this Aug 24, 2019

dzafman deleted the wip-41156 branch August 27, 2019 23:30

dzafman restored the wip-41156 branch August 30, 2019 22:43

dzafman reopened this Aug 30, 2019

dzafman closed this Sep 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dump_float() poor output #29545

dump_float() poor output #29545

dzafman commented Aug 7, 2019 •

edited

dzafman commented Aug 7, 2019

tchaikov Aug 8, 2019

batrick Aug 8, 2019

dzafman Aug 8, 2019

dzafman Aug 8, 2019

socketpair Sep 8, 2019

dzafman commented Aug 24, 2019

dzafman commented Aug 30, 2019 •

edited

dzafman commented Sep 3, 2019 •

edited

tchaikov commented Sep 7, 2019

socketpair commented Sep 7, 2019

socketpair commented Sep 8, 2019

dzafman commented Sep 9, 2019

socketpair commented Sep 9, 2019

dzafman commented Sep 16, 2019

dump_float() poor output #29545

dump_float() poor output #29545

Conversation

dzafman commented Aug 7, 2019 • edited

Checklist

dzafman commented Aug 7, 2019

tchaikov Aug 8, 2019

Choose a reason for hiding this comment

batrick Aug 8, 2019

Choose a reason for hiding this comment

dzafman Aug 8, 2019

Choose a reason for hiding this comment

ceph health

ceph daemon mgr.x dump_osd_network

dzafman Aug 8, 2019

Choose a reason for hiding this comment

socketpair Sep 8, 2019

Choose a reason for hiding this comment

dzafman commented Aug 24, 2019

dzafman commented Aug 30, 2019 • edited

dzafman commented Sep 3, 2019 • edited

tchaikov commented Sep 7, 2019

socketpair commented Sep 7, 2019

socketpair commented Sep 8, 2019

dzafman commented Sep 9, 2019

socketpair commented Sep 9, 2019

dzafman commented Sep 16, 2019

dzafman commented Aug 7, 2019 •

edited

dzafman commented Aug 30, 2019 •

edited

dzafman commented Sep 3, 2019 •

edited