Make RTCStatsReport::ToString() return JSON-parseable string.

Make RTCStatsReport::ToString() return JSON-parseable string.

BUG=chromium:653087
Review-Url: https://codereview.webrtc.org/2983243002
Cr-Commit-Position: refs/heads/master@{#19180}
Committed: https://chromium.googlesource.com/external/webrtc/+/35a872c0e6752f1c2ccd1fd15f6f721612ee180a

[ehmaldonado_webrtc, 2017-07-21 11:59:39]

Description was changed from

==========
Make RTCStatsReport::ToString() return JSON-parseable string.

BUG=
==========

to

==========
Make RTCStatsReport::ToString() return JSON-parseable string.

BUG=chromium:653087
==========

[ehmaldonado_webrtc, 2017-07-21 12:13:30]

ehmaldonado@webrtc.org changed reviewers:
+ hbos@webrtc.org

[ehmaldonado_webrtc, 2017-07-21 12:13:31]

PTAL
I'm not sure about moving the type from outside to a field called "type" inside
the dict, and replacing "undefined" with "null".
What do you think?

[ehmaldonado_webrtc, 2017-07-21 12:13:33]

PTAL
I'm not sure about moving the type from outside to a field called "type" inside
the dict, and replacing "undefined" with "null".
What do you think?

[hbos, 2017-07-24 15:00:35]

On 2017/07/21 12:13:33, ehmaldonado_webrtc wrote:
> PTAL
> I'm not sure about moving the type from outside to a field called "type"
inside
> the dict, and replacing "undefined" with "null".
> What do you think?

Isn't the type already a field inside the dict? It should be. All stats
dictionaries extend the RTCStats dictionary, so the type, id and timestamp are
plain ol' dictionary members.
They're special variables in C++ for implementation detail reasons but they
should be treated like any other RTCStats::Member().
{
  "type": "foo",
  "id": "foo1",
  "timestamp": 1234,
  "fooMember": [ 1, 2, 3 ]
}

In JavaScript and the spec there's a subtle difference between undefined and
null. Undefined fields are fields that are missing from the stats objects, which
is different than a member being null - the member doesn't exist to have the
value null in that case.
Since we don't have any stat that can take the value null yet we could say that
an undefined member is null. But as-is, this would include stats that are never
collected, and parsing would result in dictionaries with members that have the
invalid value null.
This would also mean that for track stats both audio-only and video-only stats
members would be listed, where for video tracks all the audio-only stats are
listed as null and vice versa instead of not being present.

I would prefer to exclude undefined members from the JSON object. Any stats
consumer must be prepared for the possibility of stats members being missing.
This also makes the string shorter, since a lot of members are not always
defined.

I see we have a json parser (webrtc/rtc_base/json.h including a json library and
defining helper functions). Can you add a rtcstats_unittest.cc that parses an
example dictionary? See how the other tests in that file define their own
RTCStats-derived dictionaries for testing purposes.

[ehmaldonado_webrtc, 2017-07-24 15:37:13]

> Isn't the type already a field inside the dict? It should be. All stats
> dictionaries extend the RTCStats dictionary, so the type, id and timestamp are
> plain ol' dictionary members.
> They're special variables in C++ for implementation detail reasons but they
> should be treated like any other RTCStats::Member().
> {
>   "type": "foo",
>   "id": "foo1",
>   "timestamp": 1234,
>   "fooMember": [ 1, 2, 3 ]
> }

It was printed as:
foo {
  id: foo1
  timestamp: 1234
  fooMember: {1, 2, 3}
}

So I guess there's no problem to print it as your example.

> In JavaScript and the spec there's a subtle difference between undefined and
> null. Undefined fields are fields that are missing from the stats objects,
which
> is different than a member being null - the member doesn't exist to have the
> value null in that case.
> Since we don't have any stat that can take the value null yet we could say
that
> an undefined member is null. But as-is, this would include stats that are
never
> collected, and parsing would result in dictionaries with members that have the
> invalid value null.
> This would also mean that for track stats both audio-only and video-only stats
> members would be listed, where for video tracks all the audio-only stats are
> listed as null and vice versa instead of not being present.
> 
> I would prefer to exclude undefined members from the JSON object. Any stats
> consumer must be prepared for the possibility of stats members being missing.
> This also makes the string shorter, since a lot of members are not always
> defined.

So is it ok to skip printing undefined stats? Or would you prefer to add a
ToJson method
that skips them, and keep printing them on ToString?
 
> I see we have a json parser (webrtc/rtc_base/json.h including a json library
and
> defining helper functions). Can you add a rtcstats_unittest.cc that parses an
> example dictionary? See how the other tests in that file define their own
> RTCStats-derived dictionaries for testing purposes.

Ok, will do :)

[hbos, 2017-07-24 15:50:22]

On 2017/07/24 15:37:13, ehmaldonado_webrtc wrote:
> > Isn't the type already a field inside the dict? It should be. All stats
> > dictionaries extend the RTCStats dictionary, so the type, id and timestamp
are
> > plain ol' dictionary members.
> > They're special variables in C++ for implementation detail reasons but they
> > should be treated like any other RTCStats::Member().
> > {
> >   "type": "foo",
> >   "id": "foo1",
> >   "timestamp": 1234,
> >   "fooMember": [ 1, 2, 3 ]
> > }
> 
> It was printed as:
> foo {
>   id: foo1
>   timestamp: 1234
>   fooMember: {1, 2, 3}
> }
> 
> So I guess there's no problem to print it as your example.
> 
> > In JavaScript and the spec there's a subtle difference between undefined and
> > null. Undefined fields are fields that are missing from the stats objects,
> which
> > is different than a member being null - the member doesn't exist to have the
> > value null in that case.
> > Since we don't have any stat that can take the value null yet we could say
> that
> > an undefined member is null. But as-is, this would include stats that are
> never
> > collected, and parsing would result in dictionaries with members that have
the
> > invalid value null.
> > This would also mean that for track stats both audio-only and video-only
stats
> > members would be listed, where for video tracks all the audio-only stats are
> > listed as null and vice versa instead of not being present.
> > 
> > I would prefer to exclude undefined members from the JSON object. Any stats
> > consumer must be prepared for the possibility of stats members being
missing.
> > This also makes the string shorter, since a lot of members are not always
> > defined.
> 
> So is it ok to skip printing undefined stats? Or would you prefer to add a
> ToJson method
> that skips them, and keep printing them on ToString?
>  
> > I see we have a json parser (webrtc/rtc_base/json.h including a json library
> and
> > defining helper functions). Can you add a rtcstats_unittest.cc that parses
an
> > example dictionary? See how the other tests in that file define their own
> > RTCStats-derived dictionaries for testing purposes.
> 
> Ok, will do :)

Yeah skip 'em! Pretend "undefined" members don't exist

[ehmaldonado_webrtc, 2017-07-25 08:46:16]

On 2017/07/24 15:50:22, hbos wrote:
> On 2017/07/24 15:37:13, ehmaldonado_webrtc wrote:
> > > Isn't the type already a field inside the dict? It should be. All stats
> > > dictionaries extend the RTCStats dictionary, so the type, id and timestamp
> are
> > > plain ol' dictionary members.
> > > They're special variables in C++ for implementation detail reasons but
they
> > > should be treated like any other RTCStats::Member().
> > > {
> > >   "type": "foo",
> > >   "id": "foo1",
> > >   "timestamp": 1234,
> > >   "fooMember": [ 1, 2, 3 ]
> > > }
> > 
> > It was printed as:
> > foo {
> >   id: foo1
> >   timestamp: 1234
> >   fooMember: {1, 2, 3}
> > }
> > 
> > So I guess there's no problem to print it as your example.
> > 
> > > In JavaScript and the spec there's a subtle difference between undefined
and
> > > null. Undefined fields are fields that are missing from the stats objects,
> > which
> > > is different than a member being null - the member doesn't exist to have
the
> > > value null in that case.
> > > Since we don't have any stat that can take the value null yet we could say
> > that
> > > an undefined member is null. But as-is, this would include stats that are
> > never
> > > collected, and parsing would result in dictionaries with members that have
> the
> > > invalid value null.
> > > This would also mean that for track stats both audio-only and video-only
> stats
> > > members would be listed, where for video tracks all the audio-only stats
are
> > > listed as null and vice versa instead of not being present.
> > > 
> > > I would prefer to exclude undefined members from the JSON object. Any
stats
> > > consumer must be prepared for the possibility of stats members being
> missing.
> > > This also makes the string shorter, since a lot of members are not always
> > > defined.
> > 
> > So is it ok to skip printing undefined stats? Or would you prefer to add a
> > ToJson method
> > that skips them, and keep printing them on ToString?
> >  
> > > I see we have a json parser (webrtc/rtc_base/json.h including a json
library
> > and
> > > defining helper functions). Can you add a rtcstats_unittest.cc that parses
> an
> > > example dictionary? See how the other tests in that file define their own
> > > RTCStats-derived dictionaries for testing purposes.
> > 
> > Ok, will do :)
> 
> Yeah skip 'em! Pretend "undefined" members don't exist

PTAL :)

[hbos, 2017-07-25 12:29:22]

https://codereview.webrtc.org/2983243002/diff/40001/webrtc/api/stats/rtcstats.h
File webrtc/api/stats/rtcstats.h (right):

https://codereview.webrtc.org/2983243002/diff/40001/webrtc/api/stats/rtcstats...
webrtc/api/stats/rtcstats.h:77: std::string ToString() const;
Let's rename it ToJson().

https://codereview.webrtc.org/2983243002/diff/40001/webrtc/api/stats/rtcstats...
File webrtc/api/stats/rtcstatsreport.h (right):

https://codereview.webrtc.org/2983243002/diff/40001/webrtc/api/stats/rtcstats...
webrtc/api/stats/rtcstatsreport.h:86: std::string ToString() const;
ToJson()

https://codereview.webrtc.org/2983243002/diff/40001/webrtc/stats/rtcstats_uni...
File webrtc/stats/rtcstats_unittest.cc (right):

https://codereview.webrtc.org/2983243002/diff/40001/webrtc/stats/rtcstats_uni...
webrtc/stats/rtcstats_unittest.cc:225: stats.m_sequence_string =
sequence_string;
Hmm, will this be a problem for you?
https://stackoverflow.com/questions/16809932/does-libjson-support-64-bit-int-...

It seems our json library has the types you test here, but not the
uint64_t/int64_t values:
https://cs.chromium.org/chromium/src/third_party/jsoncpp/source/include/json/...

When we surface stats to JavaScript of course we're limited by JavaScript
precision as well, and we static_cast all integer types to double since that's
how numbers are stored in the resulting objects:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/peerco...

Converting 64-bit to double may yield loss of precision. This is fine, but not
necessary in C++ land. But if JSON is we need to static_cast before ToJson.

Can you add large 64-bit values to the unittest. See if JSON can parse it large
64-bit values as double, otherwise output the 64-bit values as lossy doubles.
That might be the safest. Outputting all as double will yield the same values as
the JavaScript view of the stats.

https://codereview.webrtc.org/2983243002/diff/40001/webrtc/stats/rtcstats_uni...
webrtc/stats/rtcstats_unittest.cc:234:
EXPECT_TRUE(rtc::GetStringFromJsonObject(json_output, "mString", &m_string));
nit: you skipped mDouble

https://codereview.webrtc.org/2983243002/diff/40001/webrtc/stats/rtcstats_uni...
webrtc/stats/rtcstats_unittest.cc:268:
EXPECT_FALSE(rtc::GetIntFromJsonObject(json_output, "mInt64", &m_int64));
This is good. Keep at least one member undefined to EXPECT it to be missing,
even if the 64-bit members are used based on previous comment.

https://codereview.webrtc.org/2983243002/diff/40001/webrtc/stats/rtcstatsrepo...
File webrtc/stats/rtcstatsreport.cc (right):

https://codereview.webrtc.org/2983243002/diff/40001/webrtc/stats/rtcstatsrepo...
webrtc/stats/rtcstatsreport.cc:114: oss << ']';
Is it valid JSON to have just an array, and not a surrounding dictionary? Will
this be used for JSON parsing?

If it needs to be updated note that the stats report is a Map between stats IDs
and stats objects. Also note that this might affect the desired indentation of
the output string, if we still want to split it up into multiple lines.

Also note: the report also has a timestamp which currently is not included, but
it isn't in the spec so feel free to keep skipping it.

[ehmaldonado_webrtc, 2017-07-27 08:56:29]

PTAL

https://codereview.webrtc.org/2983243002/diff/60001/webrtc/stats/rtcstats.cc
File webrtc/stats/rtcstats.cc (right):

https://codereview.webrtc.org/2983243002/diff/60001/webrtc/stats/rtcstats.cc#...
webrtc/stats/rtcstats.cc:148: #define
WEBRTC_DEFINE_RTCSTATSMEMBER_DEFAULT_JSON(T, type, is_seq, is_str, \
I ran "git cl format" and got this. Should I revert that?

[hbos, 2017-07-27 16:27:41]

https://codereview.webrtc.org/2983243002/diff/60001/webrtc/stats/rtcstats.cc
File webrtc/stats/rtcstats.cc (right):

https://codereview.webrtc.org/2983243002/diff/60001/webrtc/stats/rtcstats.cc#...
webrtc/stats/rtcstats.cc:56: oss << std::setprecision(DBL_DIG + 1) <<
static_cast<double>(value);
nit: Hm, maybe prefer std::numeric_limits<double>::digits10 + 1? Same value, I
just prefer to avoid macros.

I don't know if this is ever applicable, but the DBL_DIG or digits10 could be
greater than expected depending on architecture. If we know JSON is 15 digits
maybe we should use our own constant that is 15 regardless?

https://codereview.webrtc.org/2983243002/diff/60001/webrtc/stats/rtcstats.cc#...
webrtc/stats/rtcstats.cc:148: #define
WEBRTC_DEFINE_RTCSTATSMEMBER_DEFAULT_JSON(T, type, is_seq, is_str, \
On 2017/07/27 08:56:29, ehmaldonado_webrtc wrote:
> I ran "git cl format" and got this. Should I revert that?

Let's keep it, it looks readable and people might do git cl format here again in
the future.

https://codereview.webrtc.org/2983243002/diff/60001/webrtc/stats/rtcstats.cc#...
webrtc/stats/rtcstats.cc:226: VectorToStringAsDouble(value_));
Can you change back the order (..., kInt32, kUint32, kInt64, kUint64, ...,
kSequenceInt32, kSequenceUint32, kSequenceInt64, kSequenceUint64, ...)?
It matches the order of the RTCStatsMemberInterface::Type enum. Optionally
remove the _DEFAULT_JSON macro to avoid the mix of enums for clarity, repeating
one argument is fine.

https://codereview.webrtc.org/2983243002/diff/60001/webrtc/stats/rtcstats_uni...
File webrtc/stats/rtcstats_unittest.cc (right):

https://codereview.webrtc.org/2983243002/diff/60001/webrtc/stats/rtcstats_uni...
webrtc/stats/rtcstats_unittest.cc:26: // ammounts to about 15.95 decimal digits.
Thus, when comparing large numbers
nit: amounts

https://codereview.webrtc.org/2983243002/diff/60001/webrtc/stats/rtcstats_uni...
webrtc/stats/rtcstats_unittest.cc:319:
EXPECT_FALSE(rtc::GetIntFromJsonObject(json_output, "mUint64", &m_uint64));
nit: The only other member not touched in this test is m_uint32, might as well
expect it to be missing too.

https://codereview.webrtc.org/2983243002/diff/60001/webrtc/stats/rtcstatsrepo...
File webrtc/stats/rtcstatsreport.cc (right):

https://codereview.webrtc.org/2983243002/diff/60001/webrtc/stats/rtcstatsrepo...
webrtc/stats/rtcstatsreport.cc:114: oss << ']';
Again, is this parseable by json? Just an array that isn't an element of a
dictionary?

Currently:
[{
  "type": "type"
  "id": "id1",
  "timestamp": 123,
},
{
  "type": "type"
  "id": "id2",
  "timestamp": 123,
}]

As opposed to:
{"stats": [{
  "type": "type"
  "id": "id1",
  "timestamp": 123,
},
{
  "type": "type"
  "id": "id2",
  "timestamp": 123,
}]}

Or perhaps more appropriately since this is a map:
{"id1": {
  "type": "type"
  "id": "id1",
  "timestamp": 123,
},
"id2": {
  "type": "type"
  "id": "id2",
  "timestamp": 123,
}}

(Or variations with other or no indentations.)

[hbos, 2017-07-27 16:29:28]

https://codereview.webrtc.org/2983243002/diff/60001/webrtc/stats/rtcstatsrepo...
File webrtc/stats/rtcstatsreport.cc (right):

https://codereview.webrtc.org/2983243002/diff/60001/webrtc/stats/rtcstatsrepo...
webrtc/stats/rtcstatsreport.cc:114: oss << ']';
On 2017/07/27 16:27:41, hbos wrote:
> Again, is this parseable by json? Just an array that isn't an element of a
> dictionary?
> 
> Currently:
> [{
>   "type": "type"
>   "id": "id1",
>   "timestamp": 123,
> },
> {
>   "type": "type"
>   "id": "id2",
>   "timestamp": 123,
> }]
> 
> As opposed to:
> {"stats": [{
>   "type": "type"
>   "id": "id1",
>   "timestamp": 123,
> },
> {
>   "type": "type"
>   "id": "id2",
>   "timestamp": 123,
> }]}
> 
> Or perhaps more appropriately since this is a map:
> {"id1": {
>   "type": "type"
>   "id": "id1",
>   "timestamp": 123,
> },
> "id2": {
>   "type": "type"
>   "id": "id2",
>   "timestamp": 123,
> }}
> 
> (Or variations with other or no indentations.)

In any case, we could get rid of any spaces and newline characters to make this
more compact if it's primarily meant for json parsers.

[ehmaldonado_webrtc, 2017-07-28 09:15:16]

PTAL.
I also think it'd be a good idea to print doubles using 15 digits.

[ehmaldonado_webrtc, 2017-07-28 09:15:43]

https://codereview.webrtc.org/2983243002/diff/60001/webrtc/stats/rtcstatsrepo...
File webrtc/stats/rtcstatsreport.cc (right):

https://codereview.webrtc.org/2983243002/diff/60001/webrtc/stats/rtcstatsrepo...
webrtc/stats/rtcstatsreport.cc:114: oss << ']';
Yes, an array is parseable by JSON.
And done. Erased spaces and newline characters

[hbos, 2017-07-28 09:30:58]

lgtm

https://codereview.webrtc.org/2983243002/diff/80001/webrtc/stats/rtcstats.cc
File webrtc/stats/rtcstats.cc (right):

https://codereview.webrtc.org/2983243002/diff/80001/webrtc/stats/rtcstats.cc#...
webrtc/stats/rtcstats.cc:22: // Produces "[ a, b, c ]". Works for non-vector
|RTCStatsMemberInterface::Type|
nit: Remove spaces from the example in the comment too.

https://codereview.webrtc.org/2983243002/diff/80001/webrtc/stats/rtcstats.cc#...
webrtc/stats/rtcstats.cc:37: // Produces "[ \"a\", \"b\", \"c\" ]". Works for
vectors of both const char* and
nit: And here.

[ehmaldonado_webrtc, 2017-07-28 13:21:26]

The CQ bit was checked by ehmaldonado@webrtc.org

[ehmaldonado_webrtc, 2017-07-28 13:21:27]

The patchset sent to the CQ was uploaded after l-g-t-m from hbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2983243002/#ps100001 (title:
"Address nit.")

[commit-bot: I haz the power, 2017-07-28 13:21:29]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-07-28 13:24:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-28 13:24:40]

Try jobs failed on following builders:
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/22359)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)

[ehmaldonado_webrtc, 2017-07-28 13:53:38]

PTAL, I had to replace ToString with ToJson in a couple other files. I was
running only rtcstats_unittests.

[hbos, 2017-07-28 13:57:46]

lgtm

[ehmaldonado_webrtc, 2017-07-28 14:02:08]

The CQ bit was checked by ehmaldonado@webrtc.org

[commit-bot: I haz the power, 2017-07-28 14:02:19]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-07-28 14:29:13]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1501250528161940,
"parent_rev": "836f60cda1775d65f233f17501f5fdad96e0fbc0", "commit_rev":
"35a872c0e6752f1c2ccd1fd15f6f721612ee180a"}

[commit-bot: I haz the power, 2017-07-28 14:29:30]

Description was changed from

==========
Make RTCStatsReport::ToString() return JSON-parseable string.

BUG=chromium:653087
==========

to

==========
Make RTCStatsReport::ToString() return JSON-parseable string.

BUG=chromium:653087

Review-Url: https://codereview.webrtc.org/2983243002
Cr-Commit-Position: refs/heads/master@{#19180}
Committed:
https://chromium.googlesource.com/external/webrtc/+/35a872c0e6752f1c2ccd1fd15...
==========

[commit-bot: I haz the power, 2017-07-28 14:29:32]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/external/webrtc/+/35a872c0e6752f1c2ccd1fd15...