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

Status: NEW

(Core :: WebRTC: Signaling, defect, P4)

Description

[Suhas]

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

[Adam Roach [:abr]]

Marking as needinformation so that I remember to look over the spreadsheet and give feedback...

Comment 2

[Adam Roach [:abr]]

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

[Adam Roach [:abr]]

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.

Comment 4

[Suhas]

Attachment: Cleanup Logging levels in MTransport and MediaPipeline

Comment 5

[Suhas]

Attachment: Log Levels cleanup for Signlaing (FullCall

Comment 6

[Suhas]

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

Comment 7

[Suhas]

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

Comment 8

[Ethan Hugg [:ehugg]]

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.

Comment 9

[Suhas]

Attachment: Log Levels cleanup for Signlaing (FullCall)

Comment 10

[Suhas]

Attachment: Log Levels cleanup for Signlaing (FullCall)

Comment 11

[Suhas]

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

Comment 12

[Suhas]

Attachment: Log Levels cleanup for Signlaing (FullCall)

Comment 13

[Suhas]

Attachment: Log Levels cleanup for Signlaing (FullCall)

Comment 14

[Ethan Hugg [:ehugg]]

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.

Comment 15

[Suhas]

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

Comment 16

[Ethan Hugg [:ehugg]]

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?

Comment 17

[Suhas]

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

Comment 18

[Suhas]

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

Comment 19

[Suhas]

Attachment: Log Levels cleanup for Signlaing (FullCall)

Comment 20

[Suhas]

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

Taking feedback+ from Ethan and asking Adam for review

Comment 21

[Adam Roach [:abr]]

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

Comment 22

[Suhas]

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

Comment 23

[Eric Rescorla (:ekr)]

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

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

Out of date.

Comment 24

[Adam Roach [:abr]]

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.

Comment 25

[Byron Campen [:bwc]]

Is that spreadsheet still around for this?

Comment 26

[Adam Roach [:abr]]

(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.

Comment 27

[Bulk Bug Changes for mreavy's org]

Mass change P3->P4 to align with new Mozilla triage process.