json with `Infinity` values is invalid #813

Closed
alexdean opened this Issue Aug 6, 2014 · 17 comments

Projects

None yet

7 participants

@alexdean
alexdean commented Aug 6, 2014

When I make a request like http://mygraphite.com/render/?target=some.metric.name&format=json, I expect to get valid JSON.

I sometimes get invalid JSON containing Infinity values.

[{"target": "some.metric.name", "datapoints": [[Infinity, 1407296460], [Infinity, 1407296520], [Infinity, 1407296580]]}]

Not sure what the correct output should be. Maybe null?

@obfuscurity
Member

Per the Python json docs:

If allow_nan is True (the default), then NaN, Infinity, and -Infinity will be encoded
as such. This behavior is not JSON specification compliant, but is consistent with 
most JavaScript based encoders and decoders. Otherwise, it will be a ValueError 
to encode such floats.

So we could adjust our use of json.dumps to override this default and make it compliant, but this seems to me like it would cause breakage with the numerous JS clients consuming the Graphite API.

Curious what use case you're seeing where this is causing breakage for you.

@alexdean
alexdean commented Aug 6, 2014

Causes a JSON parse error in Grafana. ref grafana/grafana#651

@obfuscurity
Member

Can you try setting allow_nan in the aforementioned location and see if that fixes it for you?

@alexdean
alexdean commented Aug 6, 2014

Not sure I understand. You'd like me to try setting allow_nan = False? Sounds like that will just result in a ValueError when I try to render the data.

@alexdean
alexdean commented Aug 6, 2014

The assertion that this behavior is consistent with most JavaScript based encoders and decoders seems incorrect. Infinity is a parse error in both Chrome & Firefox.

Chrome:
Chrome failing to parse Infinity

Firefox:
FF failing to parse Infinity

@obfuscurity
Member

How would you suggest that we handle these invalid values? Encode them as null?

@alexdean
alexdean commented Aug 6, 2014

Encoding as null would be fine for my immediate case. I'm not sure what other implications that might have.

@alexdean
alexdean commented Aug 6, 2014

Re: JSON parsing of Infinity... My previous examples were mistaken, though the result is the same.

Firefox:
FF can't parse 'Infinity' as JSON

Chrome:
Chrome can't parse 'Infinity' as JSON

@drawks
Member
drawks commented Aug 6, 2014

Well, I think really that the solution here should probably preserve backwards compatible behavior (at least for some period to transition if that is desirable) likely you'd want to have a queryarg like "strict=true" and that could do some combination of the following options.

  • render "inf" as "1e+9999" and "-inf" as "-1e+9999" to to overflow the parser's float type and result in proper inf and -inf at the end of decoding
  • render "nan" as false
  • render both infinity and negative infinity as "true" and "nan" as "false", leaving "null" as the correct response for no value at a given datapoint.
  • fully drop empty datapoint from the json and use "null" to represent "nan" and use true/false to represent inf/-inf respectively.

Honestly I think this is probably not a really worthwhile endevour as most json decoders aren't as strict as the native decoders in most javascript engines. AND inf, -inf, nan are all valid in javascript but technically not valid in json.

That said I'd be fine with a patch to implement some combination of the above behavior when strict json is requested. As long as the code is clean and comes with tests :)

@drawks
Member
drawks commented Aug 6, 2014

To be clear I'm 100% against changing the current default behavior, even if it isn't technically correct.

@obfuscurity
Member

@drawks I agree with you. I think @torkelo would be reasonable enough to patch Grafana to work around this issue, but I hesitate to pass the buck.

@alexdean
alexdean commented Aug 6, 2014

I completely understand maintaining backwards-compatibility.

That said, I'm not sure that it's correct to say this only affects some minority of JSON parsers. So far Python is the only language I've tried where Infinity actually decodes to the expected value.

$ irb
>> RUBY_VERSION
=> "2.1.2"
>> require 'json'
=> true
>> JSON.parse 'Infinity'
JSON::ParserError: 757: unexpected token at 'Infinity'
    from /Users/alex/.rvm/rubies/ruby-2.1.2/lib/ruby/2.1.0/json/common.rb:155:in `parse'
    from /Users/alex/.rvm/rubies/ruby-2.1.2/lib/ruby/2.1.0/json/common.rb:155:in `parse'
    from (irb):3
    from /Users/alex/.rvm/rubies/ruby-2.1.2/bin/irb:11:in `<main>'
$ php -a
Interactive shell

php > echo phpversion()."\n";
5.4.24
php > var_dump(json_decode('Infinity'));
NULL
@torkelo
Contributor
torkelo commented Feb 27, 2015

@obfuscurity its not something I can patch as I use the native json parsers built into browsers.

@syepes
syepes commented Feb 27, 2015

Does there exist any kind workaround?

@obfuscurity
Member

Workaround? No.

I think @drawks was on the right track before. I'd prefer a name like strictJson but otherwise agree with his suggestions. Anyone volunteering to submit this change? 👅

@brutasse
Member

Do we know if 3rd party dashboard actually rely on that? I'd be more in favor of fixing the issue. Most dashboards using format=json being javascript-based [citation needed], we'd better make sure JSON.parse works with the returned data :)

I'd translate +inf to 1e9999, -inf to -1e9999 and nan to null.

@falkenbt
falkenbt commented Jun 27, 2016 edited

Ping
This is hitting us as well since the codahale metrics library initializes values with -infinity, so graphs containing metrics that never got any update in the viewed timeframe will always break, which is not pretty.

@rehevkor5 rehevkor5 added a commit to rehevkor5/graphite-web that referenced this issue Sep 30, 2016
@rehevkor5 rehevkor5 Render infinite values to valid JSON values. Fixes #813
- The 'json' and 'jsonp' formats output float('inf') as 1e9999, float
('-inf') as -1e9999, and float('nan') as null
- The 'dygraph' format (a JSON-based format) outputs the same as
Infinity, -Infinity, and null (see http://dygraphs
.com/tests/gviz-infinity.html)
- Minor updates to documents to make sure commands can be run without errors or problems
e3fafa1
@obfuscurity obfuscurity pushed a commit that closed this issue Nov 29, 2016
@rehevkor5 rehevkor5 Render infinite values to valid JSON values. Fixes #813
- The 'json' and 'jsonp' formats output float('inf') as 1e9999, float
('-inf') as -1e9999, and float('nan') as null
- The 'dygraph' format (a JSON-based format) outputs the same as
Infinity, -Infinity, and null (see http://dygraphs
.com/tests/gviz-infinity.html)
- Minor updates to documents to make sure commands can be run without errors or problems
a971bcd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment