Closed Bug 965031 Opened 6 years ago Closed 6 years ago

Improve usage of levels of nsHttp log module

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: mayhemer, Assigned: mayhemer)

Details

Attachments

(1 file)

nsHttp:3 is useless (just logs requests and responses)
nsHttp:4 is the same (we don't log anything there)
nsHttp:5 is overloaded with very deep level logging like connection polling and mainly spdy content logging

I'd like to move some of the most generally useful stuff (nsHttpChannel and nsHttpTransaction) to nsHttp:4 so that I can split off what I really don't need and makes parsing/searching/highlighting logs painful.

Patrick, any objections/suggestions?
sounds good
Attached patch v1Splinter Review
A bit dirty and quick way - I realized we log on 4 already, so this is moving some of the deeper logs to level 5.  I'm changing default LOG() from LOG4() to LOG5() in some selected files.  I'm not worried about spdy since I can see you are using explicit logging level on some of the top level logs.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #8371759 - Flags: review?(mcmanus)
Attachment #8371759 - Flags: review?(mcmanus) → review+
https://hg.mozilla.org/mozilla-central/rev/1bb18f82f285
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
After unified build is turned on in default. This patch will break our logging level usage since the redefined `LOG` will propagate to other cpp files, and it'll change from time to time when we add more cpp files.
Flags: needinfo?(honzab.moz)
Those source files should be taken out from UNIFIED_SOURCE.
Moving those files out of UNIFIED_SOURCE seems too much.
Two proposals:
1. remove the redefine of `LOG` in those files and do s/LOG/LOG5
2. redefine `LOG` to `LOG4` in the end of those files so that the redefinition will not propagate to other files.
There is one more option: add the code strictly defining LOG to each .cpp file that is part of a unified build.  That's IMO the best solution.  And, I would also rename LOGn, LOG_ENABLEDn from nsHttp.h to HTTP_LOGn et al.  

I remember a bug we included a cpp file in unified source that redefined LOG to something completely else (a different module) and we lost ability to harvest the most important http logs - in release!  This should prevent it in the future.
Flags: needinfo?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #8)
> There is one more option: add the code strictly defining LOG to each .cpp
> file that is part of a unified build.  That's IMO the best solution.  And, I
> would also rename LOGn, LOG_ENABLEDn from nsHttp.h to HTTP_LOGn et al.  
> 
> I remember a bug we included a cpp file in unified source that redefined LOG
> to something completely else (a different module) and we lost ability to
> harvest the most important http logs - in release!  This should prevent it
> in the future.

sgtm, filed bug 1336295.
In addition I think we'll still need to undefine LOG at each .cpp file so that people won't forget to define LOG in newly created .cpp file.
You need to log in before you can comment on or make changes to this bug.