Cleanup the logging across mtransport and signaling for WebRTC (Full Call Scenario)

NEW
Assigned to

Status

()

P4
normal
Rank:
35
6 years ago
a year ago

People

(Reporter: snandaku, Assigned: snandaku)

Tracking

24 Branch
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

6 years ago
This bug is the continuation of the work done in the Bug 889615. The idea is to go over logging across mtransport and signaling for a full-call use-case and see if the logging is at the appropriate logging level.

A spreadsheet has been created with the first level of analysis here:
   https://docs.google.com/spreadsheet/ccc?key=0Aksfg6M2xJgudGtUU2E2X1A0aENZY3VVTmFLYmJtaHc#gid=2

Please let me know if you need access to this spreadsheet

Comment 1

6 years ago
Marking as needinformation so that I remember to look over the spreadsheet and give feedback...
Flags: needinfo?(adam)

Comment 2

6 years ago
Suhas - I'm afraid I'm not going to be able to get through the error level spreadsheet this week. I've just run out of time, and will lose most of tomorrow and Friday to travel. I'll see if I can work it in during the rather small amount of free time I might have next week.

Comment 3

5 years ago
Okay, I'm done going over the proposed log levels. I think the next step is to spin two different patches (one for mtransport, one for SIPCC) that adjust the logging levels as proposed by the spreadsheet.
Flags: needinfo?(adam)
(Assignee)

Updated

5 years ago
Assignee: nobody → snandaku
(Assignee)

Comment 4

5 years ago
Created attachment 792603 [details] [diff] [review]
Cleanup Logging levels in MTransport and MediaPipeline
(Assignee)

Comment 5

5 years ago
Created attachment 792851 [details] [diff] [review]
Log Levels cleanup for Signlaing (FullCall
(Assignee)

Comment 6

5 years ago
Comment on attachment 792851 [details] [diff] [review]
Log Levels cleanup for Signlaing (FullCall

This patch takes forward review from Adam on logging assignment modifications for signaling under full call scenarios
Attachment #792851 - Flags: review?(ethanhugg)
Attachment #792851 - Flags: review?(adam)
(Assignee)

Updated

5 years ago
Attachment #792851 - Attachment description: Log Levels cleanup for Signlaing (FullCall) WIP → Log Levels cleanup for Signlaing (FullCall
(Assignee)

Updated

5 years ago
Attachment #792603 - Attachment description: Cleanup Logging levels in MTransport and MediaPipeline (WIP) → Cleanup Logging levels in MTransport and MediaPipeline
(Assignee)

Comment 7

5 years ago
Comment on attachment 792603 [details] [diff] [review]
Cleanup Logging levels in MTransport and MediaPipeline

Review of attachment 792603 [details] [diff] [review]:
-----------------------------------------------------------------

Taking forward Ekr's review of logging levels spreadsheet for full call scenario
Attachment #792603 - Flags: review?(ekr)
Comment on attachment 792851 [details] [diff] [review]
Log Levels cleanup for Signlaing (FullCall

Review of attachment 792851 [details] [diff] [review]:
-----------------------------------------------------------------

The message says WIP, is this still work in progress?

::: media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_timers_using_select.c
@@ +792,5 @@
>          return rc;
>      }
>  
>      /* Bad application! */
> +    CPR_WARNING("%s - NULL pointer passed in.\n", fname);

Did you build this?  I don't have a CPR_WARNING in my tree.
Attachment #792851 - Flags: review?(ethanhugg) → review-
(Assignee)

Updated

5 years ago
Attachment #792851 - Attachment is obsolete: true
Attachment #792851 - Flags: review?(adam)
(Assignee)

Comment 9

5 years ago
Created attachment 793103 [details] [diff] [review]
Log Levels cleanup for Signlaing (FullCall)
(Assignee)

Comment 10

5 years ago
Created attachment 793261 [details] [diff] [review]
Log Levels cleanup for Signlaing (FullCall)
(Assignee)

Updated

5 years ago
Attachment #793103 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Comment on attachment 793261 [details] [diff] [review]
Log Levels cleanup for Signlaing (FullCall)

Review of attachment 793261 [details] [diff] [review]:
-----------------------------------------------------------------

This takes forward the review from Adam on logging levels for a full call scenarion
Attachment #793261 - Flags: review?(ethanhugg)
Attachment #793261 - Flags: review?(adam)
(Assignee)

Comment 12

5 years ago
Created attachment 793613 [details] [diff] [review]
Log Levels cleanup for Signlaing (FullCall)
(Assignee)

Updated

5 years ago
Attachment #793261 - Attachment is obsolete: true
Attachment #793261 - Flags: review?(ethanhugg)
Attachment #793261 - Flags: review?(adam)
(Assignee)

Comment 13

5 years ago
Created attachment 793627 [details] [diff] [review]
Log Levels cleanup for Signlaing (FullCall)
(Assignee)

Updated

5 years ago
Attachment #793613 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #793627 - Flags: review?(ethanhugg)
Attachment #793627 - Flags: review?(adam)
Comment on attachment 793627 [details] [diff] [review]
Log Levels cleanup for Signlaing (FullCall)

Review of attachment 793627 [details] [diff] [review]:
-----------------------------------------------------------------

f+ if issue about functions like cpr_warn is addressed.  I'm giving feedback - ABR will be your reviewer.

::: media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_stdio.c
@@ +148,5 @@
> +    if (rc <= 0) {
> +        return;
> +    }
> +
> +    CSFLogWarn("cpr", "%s", fmt_buf);

I believe you could simplify these by calling CSFLogWarnV the vararg version.  Maybe change the others to if that's not out-of-scope for this bug.  Also it's unclear to me why these are in OS-dependent code.  I see the darwin and linux ones changed, but not the Windows version for example, does that mean that Windows doesn't build?  Perhaps these should be moved to somewhere that's built for all targets.
Attachment #793627 - Flags: review?(ethanhugg) → feedback+
(Assignee)

Comment 15

5 years ago
Thanks Ethan for the feedback. 
The patch above builds successfully on all the 3 platforms. The changes were aligned to logging levels reported in the spreadsheet and similar changes around it for a full call scenario. Since we can't run signaling tests on Windows, I thought it is better not to update unless we have a way to verify then under debugging.

I am open for suggestions here.

Also to note, cleaning up the logging levels in signaling will be a continuous process as this patch verified the appropriate levels only for the full call scenario
I see now it's because didn't change the logging levels in the other targets.  Are you planning on updating the cpr_android versions to match your changes in cpr_linux for example?  Is someone changing cpr on windows going to be surprised that CPR_WARNING appears to exist but doesn't build?
(Assignee)

Comment 17

5 years ago
Following up with a quick chat on this with Ethan, let me analyze his comment on making CPR_**** Debug code non OS-Dependent.
(Assignee)

Comment 18

5 years ago
Comment on attachment 793627 [details] [diff] [review]
Log Levels cleanup for Signlaing (FullCall)

Backing off the CPR Changes for this bug. On discussing with Adam and Ethan , it was finalized that this patch will back out of CPR related logging changes and move them into a separate bug. A new bug 908323 has been created to deal with CPR Logging

I will be uploading a new patch for review on this bug minus CPR changes
Attachment #793627 - Flags: review?(adam)
(Assignee)

Comment 19

5 years ago
Created attachment 794164 [details] [diff] [review]
Log Levels cleanup for Signlaing (FullCall)
(Assignee)

Updated

5 years ago
Attachment #793627 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
Comment on attachment 794164 [details] [diff] [review]
Log Levels cleanup for Signlaing (FullCall)

Taking feedback+ from Ethan and asking Adam for review
Attachment #794164 - Flags: review?(adam)
(Assignee)

Updated

5 years ago
Summary: Cleanup the logging across mtransport and signaling for WebRTC → Cleanup the logging across mtransport and signaling for WebRTC (Full Call Scenario)
Comment on attachment 794164 [details] [diff] [review]
Log Levels cleanup for Signlaing (FullCall)

Review of attachment 794164 [details] [diff] [review]:
-----------------------------------------------------------------

r- for being incomplete.

This patch does not perform any of the conversions called for in ccapi.c.

This patch does not perform any of the conversions called for in ccapi_call.c.

This patch does not perform the conversion called for in cc_config.c.

This patch misses two of the conversions in ccprovider.c (lines 400 and 1580 in the spreadsheet).

This patch does not perform the conversion called for in ccsip_core.c.

This patch does not perform one of the conversions in ccsip_task (line 471 in the spreadsheet).

This patch does not perform any of the conversions called for in dcsm.c.

This patch misses three of the conversions in fsm.c (lines 157, 232, and 266 in the spreadsheet).

This patch does not perform the conversion indicated for fsmcac.c.

This patch does not perform any of the (thirteen!) conversions indicated for fsmdef.c.

This patch does not perform the conversion indicated for fsmxfr.c.

This patch does not perform one of the conversions called for in init.c (line 428 in the spreadsheet).

This patch does not perform two of the conversions for media_cap_tbl.c (lines 99 and 109 in the spreadsheet).

This patch does not handle any of the conversions for sm.c

This patch does not handle any of the conversions for ui.c

Formatting nits:
VideoConduit.cpp:503: Line is 81 characters long; please wrap to 80
PeerConnectionImpl.cpp:209: Line is 82 characters long; please wrap to 80
PeerConnectionImpl.cpp:296: Line is 84 characters long; please wrap to 80
cc_device_manager.c:193: Line is 81 characters long; please wrap to 80
ccprovider.c:376: Line is 95 characters long; please wrap to 80
ccprovider.c:651: Line is 107 characters long; please wrap to 80
config_parser.c:504: Line is 90 characters long; please wrap to 80
init.c:212: Line is 103 characters long; please wrap to 80
init.c:580: Line is 85 characters long; please wrap to 80
sdp_attr_access.c:1944: Line is 84 characters long; please wrap to 80
sdp_attr_access.c:2075: Line is 84 characters long; please wrap to 80
sdp_attr_access.c:4041: Line is 83 characters long; please wrap to 80
sdp_attr_access.c:4118: Line is 91 characters long; please wrap to 80
CC_SIPCCCallInfo.cpp:565: remove tab character
CC_SIPCCCallInfo.cpp:565: Line is 85 characters long; please wrap to 80
Attachment #794164 - Flags: review?(adam) → review-
(Assignee)

Comment 22

5 years ago
Hello Adam

  Many thanks for the review. I think the comments related to Logging levels has been incorporated in the patch since they correspond to #define defined in phone_debug.h rather than source file changes. Below I have attempted to provide complete explanation for each of the comment raised. Please let me know your thoughts

ccapi_c. defines CC_DEBUG_ENTRY which is in turn defined to be DEF_DEBUG. This patch already changes DEF_DEBUG from CSFLogNotice to CSFLogDebug in phone_debug.h , thus taking care of ccapi.c changes

ccapi_call.c defines DEF_DEBUG which was defined as CSFLogNotice before this patch. This patch changes DEF_DEBUG to CSFLogDebug , thus taking care of ccapi_call.c changes

config.c changes corresponds to TNP_DEBUG which has been updated to map to CSFLogDebug in phone_debug.h to print Debug messages instead of Notice messages thus completing config.c changes

cc_provider.c, ccsip_core.c, ccsip_c, dcsm.c,  defines DEF_DEBUG that was mapped to CSFLogNotice earlier. This has been updated to map to CSFLogDebug in the new patch thus taking care of the changes.

fsm.c, fsmxfr.c changes correspond to FSM_DEBUG_SM and it has been updated to map to CSFLogDebug instead of CSFLogNotice.

fsmcac.c changes correspond to DEF_DEBUG which was mapped to CSFLogNotice earlier and has been updated to map to CSFLogDebug in phone_debug.h 


init.c:552 has been changed from CSGLogError to CSFLogInfo as suggested
-            CSFLogError("common", "%s:  send UNLOAD msg to CCapp thread good",
+            CSFLogInfo("common", "%s:  send UNLOAD msg to CCapp thread good",
               fname);


media_cap_tbl.c also corresponds to DEF_DEBUG changes which are mapped to CSFLogDebug instead of CSFLogNotice.


sm.c changes also correspond to DEF_DEBUG changes.

ui.c changes correspond to TNP_DEBUG changes which has been mapped to CSFLogDebug instead of CSFLogNotice in phone_debug.h


I will provide a new patch with formatting changes if the above explanation looks appropriate.

Thanks

Updated

5 years ago
Flags: needinfo?(adam)
Comment on attachment 792603 [details] [diff] [review]
Cleanup Logging levels in MTransport and MediaPipeline

Review of attachment 792603 [details] [diff] [review]:
-----------------------------------------------------------------

Out of date.
Attachment #792603 - Flags: review?(ekr)
I think we want to revisit this completely after the sipcc work we're planning to do next week. Pulling off the needinfo until we have time to circle back around to this issue.
Flags: needinfo?(adam)
Is that spreadsheet still around for this?
Flags: needinfo?(adam)
(In reply to Byron Campen [:bwc] from comment #25)
> Is that spreadsheet still around for this?

Yes; I've added you to the editors list.
Flags: needinfo?(adam)

Updated

4 years ago
backlog: --- → webRTC+
Rank: 35
Priority: -- → P3
Mass change P3->P4 to align with new Mozilla triage process.
Priority: P3 → P4
You need to log in before you can comment on or make changes to this bug.