Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2986453002:
Trace the stats report as JSON instead of each stat separately. (Closed)
|
Created:
3 years, 5 months ago by ehmaldonado_webrtc Modified:
3 years, 4 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionTrace the stats report as JSON instead of each stat separately.
Trace the whole report as a string instead of each field on it's own. And test that the traces collected are valid.
R=tommi@webrtc.org, hbos@webrtc.org
BUG=chromium:653087
Review-Url: https://codereview.webrtc.org/2986453002
Cr-Commit-Position: refs/heads/master@{#19341}
Committed: https://chromium.googlesource.com/external/webrtc/+/80c65780e618f90369295fe5daf8075e9b374c26
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address comments. #
Total comments: 6
Patch Set 3 : Nits. #
Messages
Total messages: 24 (16 generated)
The CQ bit was checked by ehmaldonado@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by ehmaldonado@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
Improve tracing of WebRTC stats.
1. Remove traces from {send,receive}_statistics_proxy.cc
2. Trace the whole report as a string instead of each field on it's own.
3. Test that the traces collected are valid.
R=tommi@webrtc.org, hbos@webrtc.org
BUG=chromium:653087
==========
to
==========
Trace the stats report as JSON instead of each stat separately.
Trace the whole report as a string instead of each field on it's own. And test
that the traces collected are valid.
R=tommi@webrtc.org, hbos@webrtc.org
BUG=chromium:653087
==========
PTAL
https://codereview.webrtc.org/2986453002/diff/60001/webrtc/pc/rtcstats_integr... File webrtc/pc/rtcstats_integrationtest.cc (right): https://codereview.webrtc.org/2986453002/diff/60001/webrtc/pc/rtcstats_integr... webrtc/pc/rtcstats_integrationtest.cc:38: return reinterpret_cast<const unsigned char*>("\0"); All string literals are null-terminated, should this be ""? (['\0'] in memory as opposed to ['\0','\0']) https://codereview.webrtc.org/2986453002/diff/60001/webrtc/pc/rtcstats_integr... webrtc/pc/rtcstats_integrationtest.cc:84: bool report_received_; Add a constructor that default-initializes this to false, "RTCStatsTracedReport() : report_received_(false) {}" if we keep report_received_, though I think it is sufficient to use traced_report_ = "" (which is default-constructed automatically, no need for a constructor). https://codereview.webrtc.org/2986453002/diff/60001/webrtc/pc/rtcstats_integr... webrtc/pc/rtcstats_integrationtest.cc:94: &RTCStatsTracedReport::AddTraceEventHandler); Because of the usage of a global variable, it is wrong to call SetupEventTracer without safely doing Reset() in case this isn't the first time the tracer is set up. I suggest doing these two steps in a static member function of RTCStatsTracedReport instead. I view both Reset() and Get() as implementation details, the only "public" thing should be initialization and GetTracedReport(), how about: class RTCStatsTracedReport { public: static void SetupEventTracer(); static std::string GetLastTrace() { DCHECK(traced_report_); return traced_report_->last_trace_; } private: static void AddTraceEventHandler(...) { DCHECK(traced_report_); ... } static RTCStatsTracedReport* traced_report_ = nullptr; std::string last_trace_; }; SetupEventTracer() could simply reset last_trace_ to "", no need for a separate bool or Reset method.
Does this look good? https://codereview.webrtc.org/2986453002/diff/60001/webrtc/pc/rtcstats_integr... File webrtc/pc/rtcstats_integrationtest.cc (right): https://codereview.webrtc.org/2986453002/diff/60001/webrtc/pc/rtcstats_integr... webrtc/pc/rtcstats_integrationtest.cc:38: return reinterpret_cast<const unsigned char*>("\0"); On 2017/08/14 13:15:39, hbos wrote: > All string literals are null-terminated, should this be ""? (['\0'] in memory as > opposed to ['\0','\0']) Right, thanks. https://codereview.webrtc.org/2986453002/diff/60001/webrtc/pc/rtcstats_integr... webrtc/pc/rtcstats_integrationtest.cc:84: bool report_received_; On 2017/08/14 13:15:39, hbos wrote: > Add a constructor that default-initializes this to false, > "RTCStatsTracedReport() : report_received_(false) {}" if we keep > report_received_, though I think it is sufficient to use traced_report_ = "" > (which is default-constructed automatically, no need for a constructor). Done. https://codereview.webrtc.org/2986453002/diff/60001/webrtc/pc/rtcstats_integr... webrtc/pc/rtcstats_integrationtest.cc:94: &RTCStatsTracedReport::AddTraceEventHandler); On 2017/08/14 13:15:39, hbos wrote: > Because of the usage of a global variable, it is wrong to call SetupEventTracer > without safely doing Reset() in case this isn't the first time the tracer is set > up. I suggest doing these two steps in a static member function of > RTCStatsTracedReport instead. > > I view both Reset() and Get() as implementation details, the only "public" thing > should be initialization and GetTracedReport(), how about: > > class RTCStatsTracedReport { > public: > static void SetupEventTracer(); > static std::string GetLastTrace() { > DCHECK(traced_report_); > return traced_report_->last_trace_; > } > > private: > static void AddTraceEventHandler(...) { > DCHECK(traced_report_); > ... > } > > static RTCStatsTracedReport* traced_report_ = nullptr; > > std::string last_trace_; > }; > > SetupEventTracer() could simply reset last_trace_ to "", no need for a separate > bool or Reset method. Done.
lgtm https://codereview.webrtc.org/2986453002/diff/80001/webrtc/pc/rtcstats_integr... File webrtc/pc/rtcstats_integrationtest.cc (right): https://codereview.webrtc.org/2986453002/diff/80001/webrtc/pc/rtcstats_integr... webrtc/pc/rtcstats_integrationtest.cc:43: class RTCStatsTracedReport { nit: Maybe RTCStatsReportTraceListener is a more suitable name? https://codereview.webrtc.org/2986453002/diff/80001/webrtc/pc/rtcstats_integr... webrtc/pc/rtcstats_integrationtest.cc:53: static std::string& GetTracedReport() { nit: const std::string& last_trace() https://codereview.webrtc.org/2986453002/diff/80001/webrtc/pc/rtcstats_integr... webrtc/pc/rtcstats_integrationtest.cc:82: static RTCStatsTracedReport* traced_report_; nit: Can't find anything in the style guide, but I would guess static member variable goes before non-static member variables.
The CQ bit was checked by ehmaldonado@webrtc.org
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/2986453002/#ps100001 (title: "Nits.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2986453002/diff/80001/webrtc/pc/rtcstats_integr... File webrtc/pc/rtcstats_integrationtest.cc (right): https://codereview.webrtc.org/2986453002/diff/80001/webrtc/pc/rtcstats_integr... webrtc/pc/rtcstats_integrationtest.cc:43: class RTCStatsTracedReport { On 2017/08/14 15:49:23, hbos wrote: > nit: Maybe RTCStatsReportTraceListener is a more suitable name? Agree. Done. https://codereview.webrtc.org/2986453002/diff/80001/webrtc/pc/rtcstats_integr... webrtc/pc/rtcstats_integrationtest.cc:53: static std::string& GetTracedReport() { On 2017/08/14 15:49:23, hbos wrote: > nit: const std::string& last_trace() Done. https://codereview.webrtc.org/2986453002/diff/80001/webrtc/pc/rtcstats_integr... webrtc/pc/rtcstats_integrationtest.cc:82: static RTCStatsTracedReport* traced_report_; On 2017/08/14 15:49:23, hbos wrote: > nit: Can't find anything in the style guide, but I would guess static member > variable goes before non-static member variables. Done.
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1502727228810510,
"parent_rev": "7d829525aa30813118ce7c58bfb808212a449757", "commit_rev":
"80c65780e618f90369295fe5daf8075e9b374c26"}
Message was sent while issue was closed.
Description was changed from ========== Trace the stats report as JSON instead of each stat separately. Trace the whole report as a string instead of each field on it's own. And test that the traces collected are valid. R=tommi@webrtc.org, hbos@webrtc.org BUG=chromium:653087 ========== to ========== Trace the stats report as JSON instead of each stat separately. Trace the whole report as a string instead of each field on it's own. And test that the traces collected are valid. R=tommi@webrtc.org, hbos@webrtc.org BUG=chromium:653087 Review-Url: https://codereview.webrtc.org/2986453002 Cr-Commit-Position: refs/heads/master@{#19341} Committed: https://chromium.googlesource.com/external/webrtc/+/80c65780e618f90369295fe5d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/80c65780e618f90369295fe5d...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:100001) has been created in https://codereview.webrtc.org/3001683002/ by mbonadei@webrtc.org. The reason for reverting is: It breaks a downstream project.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
