Closed Bug 889615 Opened 7 years ago Closed 7 years ago

Implement Granular Logging in MediaPipeline/Media Transport

Categories

(Core :: WebRTC: Networking, defect)

24 Branch
x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: snandaku, Assigned: snandaku)

Details

Attachments

(1 file, 11 obsolete files)

There is need to get more granular logging of Mediapipeline and MediaTransport facilities. Heere is the mail from Ekr to begin with

We have all independently been discussing what log levels
to use for better resolution. Obviously, the PR_LOG levels
are really lame and I want to use different levels for MOZ_MTLOG
(and perhaps CSFLog).

As a strawman, here is the NR logging policy (from nrappkit).

In principle, you can log at any level you want. As a practical
matter, it works best if the levels mean fairly uniform things across
facilities. Otherwise, users and developers spend a lot of time
tweaking individual logging levels to get the detail level they
need. Here is what we use internally and recommend that you use unless
you have a very compelling reason not to:

 * LOG_EMERG -- Something so bad occurred that the entire system is
   going down.
 * LOG_ERR -- Something so bad happened that an app isn't going to
   function and probably is going to stop/restart.
 * LOG_WARNING -- Something bad happened, but it will not effect the
   operation of the app (at the time/place of the warning).
 * LOG_NOTICE -- Just like WARNING/ERR, but for events that may happen
   very frequently.
 * LOG_INFO -- Nothing bad happened.  For "quiet" (non-DEBUG)
   troubleshooting or general information.  Use this level for
   things like: logging startup/shutdown of apps
 * LOG_DEBUG -- Reams of stuff that should only be displayed
   when a very knowledgeable user is trying to troubleshoot a
   problem (and probably has our support on the line)

NOTICE is a special level for "chatting" logging of every "something
bad happened" event.  However, events logged at NOTICE will also be
aggregated and logged at WARNING/ERR at appropriate intervals, usually
on the order of a small number of times per minute.  In some cases,
when the event first occurs, it will be logged at both WARNING/ERR and
NOTICE, although in other cases it will be more appropriate to wait
until an event has occurred some number of times before the
WARNING/ERR is logged.
QA Contact: snandaku
Assignee: nobody → snandaku
QA Contact: snandaku
Attachment #770461 - Attachment is obsolete: true
Attachment #770462 - Attachment is obsolete: true
Attachment #770497 - Flags: review?(ekr)
Comment on attachment 770497 [details] [diff] [review]
Implement Granular Logging in MediaPipeline/Media Transport

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

I reviewed this patch on Rietveld:

Please do one patch that *just* renames the log levels from PR_LOG to
MOZ_MTLOG or whatever. Then we can discuss relevelling.


https://firefox-codereview.appspot.com/23001/diff/1/media/mtransport/logging.h
File media/mtransport/logging.h (right):

https://firefox-codereview.appspot.com/23001/diff/1/media/mtransport/logging.h#newcode32
media/mtransport/logging.h:32: #define LOG_DEBUG        5
These are already defined in syslog.h.

You need to do something like MOZ_MTLOG_EMERG....

Description:
Bug 889615: Granular logging

Please review this at https://firefox-codereview.appspot.com/23001/
Attachment #770497 - Flags: review?(ekr) → review-
Attached patch Renaming Logging Levels (obsolete) — Splinter Review
Comment on attachment 770512 [details] [diff] [review]
Renaming Logging Levels

Just renamed the logging levels
Attachment #770512 - Flags: review?(ekr)
Attachment #770497 - Attachment is obsolete: true
Attachment #770512 - Attachment is obsolete: true
Attachment #770512 - Flags: review?(ekr)
Attachment #770535 - Flags: review?(ekr)
Attachment #770535 - Attachment is obsolete: true
Attachment #770535 - Flags: review?(ekr)
Attachment #770538 - Attachment is obsolete: true
Attachment #770539 - Flags: review?(ekr)
Comment on attachment 770539 [details] [diff] [review]
Add Granular logging to Mediapipelin

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

lgtm with change below. Please check with abr.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +47,1 @@
>  #endif

This is not useful. Please remove.
Attachment #770539 - Flags: superreview?(adam)
Attachment #770539 - Flags: review?(ekr)
Attachment #770539 - Flags: review+
Comment on attachment 770539 [details] [diff] [review]
Add Granular logging to Mediapipelin

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

r- for the logging.h off-by-one issue. The patch as written shifts everything up by a level.

You missed a change on line 40 of transportlayerdtls.cpp.

transport_unittests.cpp has been updated to use the new log levels, but mediapipeline_unittest.cpp has not. Please make this consistent.

Please fix the following formatting issues:

transportflow.cpp:66: Line is 81 characters long; please wrap to 80
transportflow.cpp:113: Line is 83 characters long; please wrap to 80
transportflow.cpp:131: Line is 86 characters long; please wrap to 80
transportlayerdtls.cpp:438: Line is 83 characters long; please wrap to 80
transportlayerdtls.cpp:602: Line is 89 characters long; please wrap to 80
transportlayerdtls.cpp:611: Line is 81 characters long; please wrap to 80
transportlayerdtls.cpp:636: Line is 80 characters long; please wrap to 80
transportlayerdtls.cpp:651: Line is 80 characters long; please wrap to 80
transportlayerdtls.cpp:686: Line is 82 characters long; please wrap to 80
transportlayerdtls.cpp:846: Line is 89 characters long; please wrap to 80
MediaPipeline.cpp:42: tab character
MediaPipeline.cpp:42: trailing whitespace
MediaPipeline.cpp:1020: Line is 102 characters long; please wrap to 80
MediaPipeline.cpp:1027: Line is 86 characters long; please wrap to 80
SrtpFlow.cpp:57: Line is 83 characters long; please wrap to 80
SrtpFlow.cpp:62: Line is 83 characters long; please wrap to 80
SrtpFlow.cpp:145: Line is 83 characters long; please wrap to 80
SrtpFlow.cpp:167: Line is 85 characters long; please wrap to 80
SrtpFlow.cpp:189: Line is 84 characters long; please wrap to 80
SrtpFlow.cpp:211: Line is 86 characters long; please wrap to 80

::: media/mtransport/logging.h
@@ +27,5 @@
> +#define ML_ERROR            1
> +#define ML_WARNING          2
> +#define ML_NOTICE           3
> +#define ML_INFO             4
> +#define ML_DEBUG            5

These are off by one. If you look at prlog.h, "0" is defined as "PR_LOG_NONE," and is never logged. This is the opposite of what we want to happen for ML_EMERG.

"1" is "PR_LOG_ALWAYS", and is always printed. This is a reasonable mapping for ML_EMERG.
Attachment #770539 - Flags: superreview?(adam) → review-
Attachment #770539 - Attachment is obsolete: true
Attachment #770898 - Attachment is obsolete: true
Attachment #770911 - Attachment is obsolete: true
Attachment #770918 - Attachment is obsolete: true
Attachment #770925 - Attachment is obsolete: true
Attachment #770931 - Flags: superreview?(adam)
Comment on attachment 770931 [details] [diff] [review]
Add Granular logging to Mediapipelin

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

Looks good to me. Given the large number of changes made by this patch, I think we want a try push to make sure everything still builds okay across all the platforms:

https://tbpl.mozilla.org/?tree=Try&rev=cf1529500dc2
Attachment #770931 - Flags: superreview?(adam) → review+
Attachment #770931 - Flags: checkin?(adam)
Attachment #770931 - Flags: checkin?(adam) → checkin+
https://hg.mozilla.org/mozilla-central/rev/2a8e0a5b0da6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.