1259160 - Add file logging backend to nrappkit log

Status: NEW

(Core :: WebRTC: Networking, defect, P3)

Description

[Dan Minor [:dminor]]

Adding a nspr logging backend to the nrappkit log would make it easier to correlate logging between ICE and gecko and to get access to logs in automation.

Comment 1

[Dan Minor [:dminor]]

So this was bitrotted by the work over in Bug 1248565. I have a working patch which uses NSPR logging, but everything else is moving over to mozilla::Logging and support in automation has been dropped for NSPR logs.

If we use NSPR logging, we'll end up with a separate logfile and will have to add support back into automation for it. Using mozilla::Logging isn't an option (it's in C++, and I'm guessing we'd end up with a circular dependency anyway).

Unless you have a better suggestion, I think we might as well add a new destination which logs to a file which will allow us to grab the results in automation rather than bothering with NSPR logging.

Comment 2

[Nils Ohlmeier [:drno]]

So here is the main motivation for no longer logging nICEr messages to stderr: on Windows it is incredibly hard to get this log file!
I think that is because we remove support for stderr or Windows release builds, or something along these lines. I was only able to get nICEr logs on Windows, when starting Fx from Visual Studio. Then another terminal window gets started first and that one shows the nICEr logging. But then that window has no proper history.

I think it would be still better from an end users perspective if we could give them instructions on how to capture exactly one file (and just change the content of that file via one env variable). But two files are probably still better if the two files work in automation and on Windows release builds.

Comment 3

[Dan Minor [:dminor]]

I'll get things working logging to a file for now, that's better than no logs at all.

Recent try run is here [1]. It's working, but I need to investigate why we end up with two logs on Windows and OS X.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=5907a3fa6b2395a553a9add43efa4e69923cce53&selectedJob=19036036

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

You may run into the same problem we did with AEC traces/logs in e10s - the sandbox on some platforms prevents opening files from content code.

Comment 5

[Nico Grunbaum [:ng, @chew:mozilla.org]]

(In reply to Nils Ohlmeier [:drno] from comment #2)
> So here is the main motivation for no longer logging nICEr messages to
> stderr: on Windows it is incredibly hard to get this log file!
> I think that is because we remove support for stderr or Windows release
> builds, or something along these lines. I was only able to get nICEr logs on
> Windows, when starting Fx from Visual Studio. Then another terminal window
> gets started first and that one shows the nICEr logging. But then that
> window has no proper history.
> 
> I think it would be still better from an end users perspective if we could
> give them instructions on how to capture exactly one file (and just change
> the content of that file via one env variable). But two files are probably
> still better if the two files work in automation and on Windows release
> builds.

Nils, checkout Bug 1189223 . stderr may only be available on Windows under e10s if it is redirected to a file.

Comment 6

[Nils Ohlmeier [:drno]]

(In reply to Nico Grunbaum [:ng] from comment #5)
> Nils, checkout Bug 1189223 . stderr may only be available on Windows under
> e10s if it is redirected to a file.

Well that is developers. Try downloading an official Firefox release on Win and get it to log anything from nICEr. As said before I failed with several attempts. And this impacts us as I currently don't know how I request this in bug reports from users (going to tell them to compile Firefox locally first is not really an option).

Comment 7

[Nico Grunbaum [:ng, @chew:mozilla.org]]

(In reply to Nils Ohlmeier [:drno] from comment #6)
> Well that is developers.
Yeah, I am not suggesting it as a fix, just providing a pointer out why stderr logs may not be showing up.

Comment 8

[Dan Minor [:dminor]]

Hi Bob,

Not sure if you are the right person to ask, but as Randell pointed out above, this won't work in some content sandboxes. Is there any chance of getting an r+ on a whitelist entry based upon the value of a user set environment variable ["R_LOG_FILE"]?

My initial use case was to get access to this log on try pushes, where we can hack around this if required, but as Nils mentioned, it would be nice to be able to have end users generate logs for us as well.

Thanks!

Comment 9

[Bob Owen (:bobowen)]

(In reply to Dan Minor [:dminor] from comment #8)
> Hi Bob,
> 
> Not sure if you are the right person to ask, but as Randell pointed out
> above, this won't work in some content sandboxes. Is there any chance of
> getting an r+ on a whitelist entry based upon the value of a user set
> environment variable ["R_LOG_FILE"]?

On Windows, we already have a rule set up for NSPR_LOG_FILE, is that not what you need here?

If this is for DEBUG builds then, on Windows at least, we give access to the %TEMP% directory.

Otherwise, if you can use "TmpD" (NS_OS_TEMP_DIR) in the content process this is changed to point to a writeable temp directory, although this gets cleared on shutdown.

Comment 10

[Dan Minor [:dminor]]

Thanks! My original intention was to use NSPR_LOG_FILE, but then people were removing support for NSPR logging (as opposed to mozilla::Logging) from the test harnesses, so I thought it would end up being cleaner to log to a separate file.

Now I'm thinking it will be better to add the NSPR logging support back to the test harnesses rather than deal with the sandboxing for a separate file.

Comment 11

[Bob Owen (:bobowen)]

(In reply to Dan Minor [:dminor] from comment #10)
> Thanks! My original intention was to use NSPR_LOG_FILE, but then people were
> removing support for NSPR logging (as opposed to mozilla::Logging) from the
> test harnesses, so I thought it would end up being cleaner to log to a
> separate file.
> 
> Now I'm thinking it will be better to add the NSPR logging support back to
> the test harnesses rather than deal with the sandboxing for a separate file.

OK - let me know if the NSPR logging is going to turn into something else, because we should probably be adding the rule for that and maybe removing the NSPR one ... eventually.

Comment 12

[Dan Minor [:dminor]]

As it turns out, my patch to add NSPR logging to the nrappkit library only works properly in non-e10s mode. Likely it is getting truncated at some point.

Comment 13

[Dan Minor [:dminor]]

Randell, it looks like both NSPR and file logging will have their downsides. Any preference as to which one I pursue? Thanks!

Comment 14

[Dan Minor [:dminor]]

Discussed this in the webrtc standup this morning, sounds like that once AEC logs are working (bug 1269930 or follow on bugs) we can reuse that work to get a filed based log working here.

I'm going to lower this to a P2 because of this dependency.

Comment 15

[Dan Minor [:dminor]]

Unlikely I'll get back to this anytime soon.

Comment 16

[Bulk Bug Changes for mreavy's org]

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