Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2975793002:
Trace stats in RTCStatsCollector. (Closed)
|
Created:
3 years, 5 months ago by ehmaldonado_webrtc Modified:
3 years, 5 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionTrace stats in RTCStatsCollector.
BUG=chromium:653087
Review-Url: https://codereview.webrtc.org/2975793002
Cr-Commit-Position: refs/heads/master@{#19069}
Committed: https://chromium.googlesource.com/external/webrtc/+/a26196bc65bbc627ad4520070ebbb896b95708c8
Patch Set 1 #
Total comments: 3
Patch Set 2 : Address Henrik's comments. #
Total comments: 2
Patch Set 3 : Include dict name in the trace name. #
Total comments: 4
Patch Set 4 : Address comments. #
Total comments: 2
Patch Set 5 : Avoid using ostringstream. #
Total comments: 6
Patch Set 6 : Use rtc::sprintf. #Patch Set 7 : Add comment explaining purpose and usage. #
Total comments: 3
Patch Set 8 : Trace everything under webrtc_stats. #
Total comments: 5
Patch Set 9 : Rebase. Use '.' instead of '+' as separator. #Messages
Total messages: 42 (10 generated)
Description was changed from ========== [WIP] Trace stats in RTCStatsCollector. BUG= ========== to ========== [WIP] Trace stats in RTCStatsCollector. BUG=chromium:653087 ==========
Patchset #1 (id:1) has been deleted
ehmaldonado@webrtc.org changed reviewers: + hbos@webrtc.org, tommi@webrtc.org
The idea is that the stats get traced when we call getStats() in javascript.
the approach seems OK to me. https://codereview.webrtc.org/2975793002/diff/20001/webrtc/pc/rtcstatscollect... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/20001/webrtc/pc/rtcstatscollect... webrtc/pc/rtcstatscollector.cc:435: TRACE_EVENT2("webrtc_stats", "sent_frame_width", It would be good to add some context for why these are being traced here and how it can/should be used. Same below (you could refer to a comment here from the other places)
What is the purpose of tracing and is there CPU overhead to doing so? https://codereview.webrtc.org/2975793002/diff/20001/webrtc/pc/rtcstatscollect... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/20001/webrtc/pc/rtcstatscollect... webrtc/pc/rtcstatscollector.cc:494: "ssrc", video_receiver_info.ssrc()); The "ssrc" is not a identifier of this report, you should use id() instead, and dictionary type is probably of interest. See dictionaries in spec https://w3c.github.io/webrtc-stats/#stats-dictionaries or rtcstats_objects.h. See other comment, I think you want to trace by these categories: getStats API / stats dictionary type / stats object id / member name / member value e.g. "getStats" / "track" / "xxx" / "famesDropped" / "10" https://codereview.webrtc.org/2975793002/diff/20001/webrtc/pc/rtcstatscollect... webrtc/pc/rtcstatscollector.cc:778: track_to_id_.clear(); Reports are produced asynchronously on different threads as "partial reports" and merged into the final stats report containing all the stats. On this line, |cached_report_| is the result of a newly completed getStats call. If the point of tracing is to deliver a the values of all stats that are calculated you can loop through cached_report_ here instead of manually adding tracing information to each member explicitly. for (const RTCStats& stats : *cached_report_) { for (const RTCStatsMemberInterface* member : stats.Members()) { if (member->is_defined()) { ... } else { // if a member does not have a value set it is said to be undefined // you could trace it as "undefined" or you could skip tracing it. } } } stats.type() is the dictionary type, e.g. "track" for RTCMediaStreamTrackStats[1]. stats.id() is a string id that uniquely identifies this stats object (id is same across multiple getStats call if the stats are for the same component). member is a dictionary member of the stats, this is a key-value. See name(), ValueToString() and cast_to() You can a member to RTCStatsMember<T> where T is the value type corresponding to type() (e.g. uint32_t or std::vector<std::string>) if you don't want to lose type-information, otherwise just ValueToString This gives you all stats values. The stack trace is the same for all of them though, but if you want to measure call time then you should probably trace ProducePartialResultsOn...Thread methods instead, or the lower layer components that do the actual stats collecting (that would be complicated) that this file copies from. [1] https://w3c.github.io/webrtc-stats/#rtcstatstype-str*
PTAL Tommi: I'll add the comment before submitting :) Herik: Thanks for the review! Regarding CPU overhead [1]: Trace macros are very low overhead. When tracing is not turned on, trace macros cost at most a few dozen clocks. When running, trace macros cost a few thousand clocks at most. Arguments to the trace macro are evaluated only when tracing is on --- if tracing is off, the value of the arguments dont get computed. Regarding the purpose: For our performance tests in Chrome we can only compute metrics from the traces we collect, so if we want to collect and report the getStats metrics we need to trace them first. (We used to be able to get the result from JavaScript, but not anymore). I compiled Chrome with these traces and run a loopback call for 3 min. You can see the traces collected at [2]. [1] https://www.chromium.org/developers/how-tos/trace-event-profiling-tool/tracin... [2] https://drive.google.com/open?id=0B64HUqgWWN_ZRnZNa2dKT01mX2c
https://codereview.webrtc.org/2975793002/diff/40001/webrtc/pc/rtcstatscollect... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/40001/webrtc/pc/rtcstatscollect... webrtc/pc/rtcstatscollector.cc:760: stats.type(), stats.id()); I think stats name might be wrong to have as trace name, because there are a few cases where the same member name is used inside different stats dictionaries even though they're not examples of the same thing. (Example: bytesSent is in RTCSentRTPStreamStats, RTCDataChannelStats, RTCTransportStats and RTCIceCandidatePairStats). Probably, the dictionary should be the name argument, and the additional arguments be the sub-dictionary information, from higher level information to lower level. TRACE_EVENT_INSTANT2( "webrtc_stats", // category stats.type(), // name "id", // arg1_name stats.id(), // arg1_value member->name(), // arg2_name member->ValueToString()); // arg2_value Or if "name" should fully identify an example of this particular metric and not just the dictionary, then this should probably be the way to go: TRACE_EVENT_INSTANT2( "webrtc_stats", // category stats.type() + "." + member->name(), // name "id", // arg1_name stats.id(), // arg1_value "value", // arg2_name member->ValueToString()); // arg2_value
PTAL :) https://codereview.webrtc.org/2975793002/diff/40001/webrtc/pc/rtcstatscollect... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/40001/webrtc/pc/rtcstatscollect... webrtc/pc/rtcstatscollector.cc:760: stats.type(), stats.id()); On 2017/07/12 11:26:51, hbos wrote: > I think stats name might be wrong to have as trace name, because there are a few > cases where the same member name is used inside different stats dictionaries > even though they're not examples of the same thing. (Example: bytesSent is in > RTCSentRTPStreamStats, RTCDataChannelStats, RTCTransportStats and > RTCIceCandidatePairStats). Probably, the dictionary should be the name argument, > and the additional arguments be the sub-dictionary information, from higher > level information to lower level. > > TRACE_EVENT_INSTANT2( > "webrtc_stats", // category > stats.type(), // name > "id", // arg1_name > stats.id(), // arg1_value > member->name(), // arg2_name > member->ValueToString()); // arg2_value > > Or if "name" should fully identify an example of this particular metric and not > just the dictionary, then this should probably be the way to go: > > TRACE_EVENT_INSTANT2( > "webrtc_stats", // category > stats.type() + "." + member->name(), // name > "id", // arg1_name > stats.id(), // arg1_value > "value", // arg2_name > member->ValueToString()); // arg2_value Right. Done.
https://codereview.webrtc.org/2975793002/diff/60001/webrtc/pc/rtcstatscollect... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/60001/webrtc/pc/rtcstatscollect... webrtc/pc/rtcstatscollector.cc:759: trace_name << stats.type() << '.' << member->ValueToString(); If you place this in a helper function that is only invoked in the TRACE_EVENT_INSTANT2 macro arguments then you don't end up creating all these strings for naught if the trace is disabled. That or #ifdef-ing the whole for-loop to match "if tracing enabled". Speaking of which, how is it decided if this will be enabled or disabled? https://codereview.webrtc.org/2975793002/diff/60001/webrtc/pc/rtcstatscollect... webrtc/pc/rtcstatscollector.cc:762: "id", stats.id()); Does the order of "value" and "id" matter? I just remembered, there's also stats.timestamp() which is when the stats object was last updated. But there's no TRACE_EVENT_INSTANT3. If that is of interest you could add a separate trace for the timestamp on a per |stats| basis.
https://codereview.webrtc.org/2975793002/diff/60001/webrtc/pc/rtcstatscollect... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/60001/webrtc/pc/rtcstatscollect... webrtc/pc/rtcstatscollector.cc:759: trace_name << stats.type() << '.' << member->ValueToString(); On 2017/07/12 13:01:15, hbos wrote: > If you place this in a helper function that is only invoked in the > TRACE_EVENT_INSTANT2 macro arguments then you don't end up creating all these > strings for naught if the trace is disabled. > > That or #ifdef-ing the whole for-loop to match "if tracing enabled". > > Speaking of which, how is it decided if this will be enabled or disabled? Done. I don't know how it's decided, there are several layers of macros that I haven't fully understood. https://codereview.webrtc.org/2975793002/diff/60001/webrtc/pc/rtcstatscollect... webrtc/pc/rtcstatscollector.cc:762: "id", stats.id()); On 2017/07/12 13:01:15, hbos wrote: > Does the order of "value" and "id" matter? > > I just remembered, there's also stats.timestamp() which is when the stats object > was last updated. But there's no TRACE_EVENT_INSTANT3. If that is of interest > you could add a separate trace for the timestamp on a per |stats| basis. The order doesn't matter. The timestamp is not of interest for now.
lgtm
On 2017/07/12 13:49:43, hbos wrote: > lgtm Performance is measured here by the way https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/webrtc_stats..., hoping this doesn't have a significant impact on it.
https://codereview.webrtc.org/2975793002/diff/80001/webrtc/pc/rtcstatscollect... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/80001/webrtc/pc/rtcstatscollect... webrtc/pc/rtcstatscollector.cc:36: std::ostringstream oss; please don't use stringstream for anything else than debug logging. Use StringPrintf instead.
https://codereview.webrtc.org/2975793002/diff/80001/webrtc/pc/rtcstatscollect... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/80001/webrtc/pc/rtcstatscollect... webrtc/pc/rtcstatscollector.cc:36: std::ostringstream oss; On 2017/07/12 13:56:49, tommi (wëbrtc) wrote: > please don't use stringstream for anything else than debug logging. Use > StringPrintf instead. What about something like this? It looks like StringPrintf is a Chromium function, I couldn't find a definition in WebRTC.
https://codereview.webrtc.org/2975793002/diff/100001/webrtc/pc/rtcstatscollec... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/100001/webrtc/pc/rtcstatscollec... webrtc/pc/rtcstatscollector.cc:36: return std::string(stats_type) + '.' + member_name; '.' + member_name by itself is the same as doing |46 + member_name|, so are you counting on operator order and that the first operator yields a std::string, which will take override the last + operator? If that's the case, I would prefer to make it more explicit. Other than that, the general idea seems right.
https://codereview.webrtc.org/2975793002/diff/100001/webrtc/pc/rtcstatscollec... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/100001/webrtc/pc/rtcstatscollec... webrtc/pc/rtcstatscollector.cc:36: return std::string(stats_type) + '.' + member_name; On 2017/07/12 15:34:20, tommi (wëbrtc) wrote: > '.' + member_name > > by itself is the same as doing |46 + member_name|, so are you counting on > operator order and that the first operator yields a std::string, which will take > override the last + operator? > > If that's the case, I would prefer to make it more explicit. Other than that, > the general idea seems right. Yes, I'm counting on that. How would I make it more explicit? Maybe (std::string(stats_type) + '.') + member_name?
https://codereview.webrtc.org/2975793002/diff/100001/webrtc/pc/rtcstatscollec... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/100001/webrtc/pc/rtcstatscollec... webrtc/pc/rtcstatscollector.cc:36: return std::string(stats_type) + '.' + member_name; On 2017/07/12 15:40:32, ehmaldonado_webrtc wrote: > On 2017/07/12 15:34:20, tommi (wëbrtc) wrote: > > '.' + member_name > > > > by itself is the same as doing |46 + member_name|, so are you counting on > > operator order and that the first operator yields a std::string, which will > take > > override the last + operator? > > > > If that's the case, I would prefer to make it more explicit. Other than that, > > the general idea seems right. > > Yes, I'm counting on that. > How would I make it more explicit? > Maybe (std::string(stats_type) + '.') + member_name? Yes, that's one way. Alternatively you could use rtc::sprintfn and reduce the number of heap allocations to 1 (instead of possibly 3): char buffer[1024]; // assuming this would always be big enough? rtc::sprintfn(&buffer[0], sizeof(buffer), "%s.%s", stats_type, member_name); return buffer;
https://codereview.webrtc.org/2975793002/diff/100001/webrtc/pc/rtcstatscollec... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/100001/webrtc/pc/rtcstatscollec... webrtc/pc/rtcstatscollector.cc:36: return std::string(stats_type) + '.' + member_name; On 2017/07/12 15:44:52, tommi (wëbrtc) wrote: > On 2017/07/12 15:40:32, ehmaldonado_webrtc wrote: > > On 2017/07/12 15:34:20, tommi (wëbrtc) wrote: > > > '.' + member_name > > > > > > by itself is the same as doing |46 + member_name|, so are you counting on > > > operator order and that the first operator yields a std::string, which will > > take > > > override the last + operator? > > > > > > If that's the case, I would prefer to make it more explicit. Other than > that, > > > the general idea seems right. > > > > Yes, I'm counting on that. > > How would I make it more explicit? > > Maybe (std::string(stats_type) + '.') + member_name? > > Yes, that's one way. > > Alternatively you could use rtc::sprintfn and reduce the number of heap > allocations to 1 (instead of possibly 3): > > char buffer[1024]; // assuming this would always be big enough? > rtc::sprintfn(&buffer[0], sizeof(buffer), > "%s.%s", stats_type, member_name); > return buffer; Ok, I see :) Something it's not clear to me, Is it still valid to reference buffer once the function ends? Isn't it allocated for the duration of the function?
https://codereview.webrtc.org/2975793002/diff/100001/webrtc/pc/rtcstatscollec... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/100001/webrtc/pc/rtcstatscollec... webrtc/pc/rtcstatscollector.cc:36: return std::string(stats_type) + '.' + member_name; On 2017/07/12 15:58:27, ehmaldonado_webrtc wrote: > On 2017/07/12 15:44:52, tommi (wëbrtc) wrote: > > On 2017/07/12 15:40:32, ehmaldonado_webrtc wrote: > > > On 2017/07/12 15:34:20, tommi (wëbrtc) wrote: > > > > '.' + member_name > > > > > > > > by itself is the same as doing |46 + member_name|, so are you counting on > > > > operator order and that the first operator yields a std::string, which > will > > > take > > > > override the last + operator? > > > > > > > > If that's the case, I would prefer to make it more explicit. Other than > > that, > > > > the general idea seems right. > > > > > > Yes, I'm counting on that. > > > How would I make it more explicit? > > > Maybe (std::string(stats_type) + '.') + member_name? > > > > Yes, that's one way. > > > > Alternatively you could use rtc::sprintfn and reduce the number of heap > > allocations to 1 (instead of possibly 3): > > > > char buffer[1024]; // assuming this would always be big enough? > > rtc::sprintfn(&buffer[0], sizeof(buffer), > > "%s.%s", stats_type, member_name); > > return buffer; > > Ok, I see :) > > Something it's not clear to me, Is it still valid to reference buffer once the > function ends? Isn't it allocated for the duration of the function? It would indeed not be valid to reference it once the function ends. But because of the return type of our helper function "return buffer;" is implicitly the same as "return std::string(buffer);". It would copy the characters up to the first null terminating character into the std::string object that is returned. Stats dictionaries and member names are quite short and would combined always fit on much less than a single line. An 80 char buffer would have plenty of margin.
Patchset #6 (id:120001) has been deleted
PTAL Also, what do you think of the comment I added? Is it clear enough? https://codereview.webrtc.org/2975793002/diff/100001/webrtc/pc/rtcstatscollec... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/100001/webrtc/pc/rtcstatscollec... webrtc/pc/rtcstatscollector.cc:36: return std::string(stats_type) + '.' + member_name; On 2017/07/12 16:20:15, hbos wrote: > On 2017/07/12 15:58:27, ehmaldonado_webrtc wrote: > > On 2017/07/12 15:44:52, tommi (wëbrtc) wrote: > > > On 2017/07/12 15:40:32, ehmaldonado_webrtc wrote: > > > > On 2017/07/12 15:34:20, tommi (wëbrtc) wrote: > > > > > '.' + member_name > > > > > > > > > > by itself is the same as doing |46 + member_name|, so are you counting > on > > > > > operator order and that the first operator yields a std::string, which > > will > > > > take > > > > > override the last + operator? > > > > > > > > > > If that's the case, I would prefer to make it more explicit. Other than > > > that, > > > > > the general idea seems right. > > > > > > > > Yes, I'm counting on that. > > > > How would I make it more explicit? > > > > Maybe (std::string(stats_type) + '.') + member_name? > > > > > > Yes, that's one way. > > > > > > Alternatively you could use rtc::sprintfn and reduce the number of heap > > > allocations to 1 (instead of possibly 3): > > > > > > char buffer[1024]; // assuming this would always be big enough? > > > rtc::sprintfn(&buffer[0], sizeof(buffer), > > > "%s.%s", stats_type, member_name); > > > return buffer; > > > > Ok, I see :) > > > > Something it's not clear to me, Is it still valid to reference buffer once the > > function ends? Isn't it allocated for the duration of the function? > > It would indeed not be valid to reference it once the function ends. But because > of the return type of our helper function "return buffer;" is implicitly the > same as "return std::string(buffer);". It would copy the characters up to the > first null terminating character into the std::string object that is returned. > > Stats dictionaries and member names are quite short and would combined always > fit on much less than a single line. An 80 char buffer would have plenty of > margin. Makes sense. Done.
https://codereview.webrtc.org/2975793002/diff/160001/webrtc/pc/rtcstatscollec... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/160001/webrtc/pc/rtcstatscollec... webrtc/pc/rtcstatscollector.cc:38: rtc::sprintfn(&buffer[0], sizeof(buffer), "%s.%s", stats_type, member_name); You might want to add dchecks for the length of the constants https://codereview.webrtc.org/2975793002/diff/160001/webrtc/pc/rtcstatscollec... webrtc/pc/rtcstatscollector.cc:771: stats.type(), member->name()).c_str(), Have you checked if this is a concern? https://cs.chromium.org/chromium/src/base/trace_event/common/trace_event_comm...
https://codereview.webrtc.org/2975793002/diff/160001/webrtc/pc/rtcstatscollec... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/160001/webrtc/pc/rtcstatscollec... webrtc/pc/rtcstatscollector.cc:771: stats.type(), member->name()).c_str(), On 2017/07/12 16:53:03, tommi (wëbrtc) wrote: > Have you checked if this is a concern? > https://cs.chromium.org/chromium/src/base/trace_event/common/trace_event_comm... Right, I missed that. :( I think the easiest way would be to trace all events under a single trace name, something like: TRACE_EVENT_INSTANT2("webrtc_stats", "webrtc_stats", "value", member->ValueToString(), "stat_info", "%s.%s.%s" % ( stats.name(), member->name(), stats.id())); (That's not valid C++, but you get the idea :P) Otherwise I think we'll have to trace each stat separately, or define lots of const char* so we can reference them. What do you think? Maybe I'm missing something.
On 2017/07/12 18:57:10, ehmaldonado_webrtc wrote: > https://codereview.webrtc.org/2975793002/diff/160001/webrtc/pc/rtcstatscollec... > File webrtc/pc/rtcstatscollector.cc (right): > > https://codereview.webrtc.org/2975793002/diff/160001/webrtc/pc/rtcstatscollec... > webrtc/pc/rtcstatscollector.cc:771: stats.type(), member->name()).c_str(), > On 2017/07/12 16:53:03, tommi (wëbrtc) wrote: > > Have you checked if this is a concern? > > > https://cs.chromium.org/chromium/src/base/trace_event/common/trace_event_comm... > > Right, I missed that. :( > > I think the easiest way would be to trace all events under a single trace name, > something like: > > TRACE_EVENT_INSTANT2("webrtc_stats", "webrtc_stats", > "value", member->ValueToString(), > "stat_info", "%s.%s.%s" % ( > stats.name(), member->name(), stats.id())); > > (That's not valid C++, but you get the idea :P) > > Otherwise I think we'll have to trace each stat separately, or define lots of > const char* so we can reference them. > > What do you think? Maybe I'm missing something. Yes that approach should work. Incidentally also avoids some string copying :)
On 2017/07/12 19:22:15, tommi (wëbrtc) wrote: > Yes that approach should work. Incidentally also avoids some string copying :) So you like the easy approach? :) And I think we'll still have to copy something when doing "%s.%s.%s" % (stats.name(), member->name(), stats.id())
On 2017/07/12 19:36:54, ehmaldonado_webrtc wrote: > On 2017/07/12 19:22:15, tommi (wëbrtc) wrote: > > Yes that approach should work. Incidentally also avoids some string copying :) > > So you like the easy approach? :) > And I think we'll still have to copy something when doing "%s.%s.%s" % > (stats.name(), member->name(), stats.id()) oh, right. at least we won't create copies of the static strings on the heap, only the values.
PTAL :)
lgtm
https://codereview.webrtc.org/2975793002/diff/180001/webrtc/pc/rtcstatscollec... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/180001/webrtc/pc/rtcstatscollec... webrtc/pc/rtcstatscollector.cc:35: const int kStatTypeMemberNameAndIdMaxLen = 120; Henrik, do you think 120 chars will be enough?
lgtm. Update the CL description (remove the [WIP]) before landing. https://codereview.webrtc.org/2975793002/diff/180001/webrtc/pc/rtcstatscollec... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/180001/webrtc/pc/rtcstatscollec... webrtc/pc/rtcstatscollector.cc:35: const int kStatTypeMemberNameAndIdMaxLen = 120; On 2017/07/13 09:40:42, ehmaldonado_webrtc wrote: > Henrik, do you think 120 chars will be enough? Yes. In the very unlikely case that it won't be in the future we'll notice thanks to the DCHECK. https://codereview.webrtc.org/2975793002/diff/180001/webrtc/pc/rtcstatscollec... webrtc/pc/rtcstatscollector.cc:776: "type+name+id", GetStatTypeMemberNameAndId( nit: How about "type.name.id" to match the format of GetStatTypeMemberNameAndId? Or if the '.' has special meaning somewhere we could use a different separator like ' '. type, name and ids so far only use A-Z, a-z, _, -, 0-9, anything else could be a separator.
Description was changed from ========== [WIP] Trace stats in RTCStatsCollector. BUG=chromium:653087 ========== to ========== Trace stats in RTCStatsCollector. BUG=chromium:653087 ==========
Patchset #9 (id:200001) has been deleted
The CQ bit was checked by ehmaldonado@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org, hbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/2975793002/#ps220001 (title: "Rebase. Use '.' instead of '+' as separator.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2975793002/diff/180001/webrtc/pc/rtcstatscollec... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/180001/webrtc/pc/rtcstatscollec... webrtc/pc/rtcstatscollector.cc:35: const int kStatTypeMemberNameAndIdMaxLen = 120; On 2017/07/18 08:55:08, hbos wrote: > On 2017/07/13 09:40:42, ehmaldonado_webrtc wrote: > > Henrik, do you think 120 chars will be enough? > > Yes. In the very unlikely case that it won't be in the future we'll notice > thanks to the DCHECK. Oh sorry, I forgot about the length of the ID, and while typically short the RTCCertificate IDs are a special case where it's really long! We should make the buffer longer. I suspect the DCHECK will fail on rtcstats_integrationtest.cc. Example string for certificates: certificate.fingerprintAlgorithm.FE:C9:98:64:F3:3C:3E:A2:1A:ED:00:2D:17:18:6A:F2:2D:27:E5:BC:89:DE:67:11:83:B6:D6:5D:40:5E:FB:A3 That's 128 characters long! Let's go ahead and make kStatTypeMemberNameAndIdMaxLen = 512 for plenty of margin (which is fine since it's on the stack).
https://codereview.webrtc.org/2975793002/diff/180001/webrtc/pc/rtcstatscollec... File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2975793002/diff/180001/webrtc/pc/rtcstatscollec... webrtc/pc/rtcstatscollector.cc:35: const int kStatTypeMemberNameAndIdMaxLen = 120; On 2017/07/18 10:09:48, hbos wrote: > On 2017/07/18 08:55:08, hbos wrote: > > On 2017/07/13 09:40:42, ehmaldonado_webrtc wrote: > > > Henrik, do you think 120 chars will be enough? > > > > Yes. In the very unlikely case that it won't be in the future we'll notice > > thanks to the DCHECK. > > Oh sorry, I forgot about the length of the ID, and while typically short the > RTCCertificate IDs are a special case where it's really long! We should make the > buffer longer. I suspect the DCHECK will fail on rtcstats_integrationtest.cc. > > Example string for certificates: > > certificate.fingerprintAlgorithm.FE:C9:98:64:F3:3C:3E:A2:1A:ED:00:2D:17:18:6A:F2:2D:27:E5:BC:89:DE:67:11:83:B6:D6:5D:40:5E:FB:A3 > > That's 128 characters long! Let's go ahead and make > kStatTypeMemberNameAndIdMaxLen = 512 for plenty of margin (which is fine since > it's on the stack). Or rather "certificate.fingerprintAlgorithm.RTCCertificate_FE:C9:98:64:F3:3C:3E:A2:1A:ED:00:2D:17:18:6A:F2:2D:27:E5:BC:89:DE:67:11:83:B6:D6:5D:40:5E:FB:A3", even longer. (And yeah, ':' can't be separator either.)
The bots aren't failing. Maybe the tracing isn't enabled in the integrationtest?
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1500371748056250,
"parent_rev": "16005b77839e8d2af95405e6434a7fbdbf4b5851", "commit_rev":
"a26196bc65bbc627ad4520070ebbb896b95708c8"}
Message was sent while issue was closed.
Description was changed from ========== Trace stats in RTCStatsCollector. BUG=chromium:653087 ========== to ========== Trace stats in RTCStatsCollector. BUG=chromium:653087 Review-Url: https://codereview.webrtc.org/2975793002 Cr-Commit-Position: refs/heads/master@{#19069} Committed: https://chromium.googlesource.com/external/webrtc/+/a26196bc65bbc627ad4520070... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:220001) as https://chromium.googlesource.com/external/webrtc/+/a26196bc65bbc627ad4520070... |
