nvaccess / nvda

NVDA, the free and open source Screen Reader for Microsoft Windows
https://www.nvaccess.org/
Other
2.11k stars 637 forks source link

Feature change: Set log level to debug on Alpha (& try build) snapshots #8880

Open Qchristensen opened 6 years ago

Qchristensen commented 6 years ago

Currently, the log level defaults to "Info" on any temporary copy of NVDA (setup file, etc). Since the point of alpha and try builds is problem-solving and testing, it would be useful to have the log level set to debug by default on alpha and try builds only. On RC and stable builds definitely info log level is fine (and I could be swayed either way on "Beta" builds).

This would also allow easier debugging of things like startup issues as "restart with debug logging enabled" does not work on temporary copies (ie where you run from the setup file).

Tested with latest stable and alpha builds and on Windows 10, but as it is a default setting change, it would affect any system.

LeonarddeR commented 6 years ago

This would mean that the default config spec differs for try/alpha builds and other builds. This sounds a bit complex to accomplish. I'd say we certainly do not want to enforce debug logging upon Alpha builds.

Qchristensen commented 6 years ago

Ok, I hadn't looked into the complexity of it, I was considering it from the difficulty of getting a debug log from an alpha tester (running alpha portable, and stable installed) who is having a startup issue with NVDA.

In that case though, what if I update the issue to allowing the --debug-logging

Just out of curiosity, re starting with a different config spec for alpha and stable, couldn't the same be argued for having an error sound for try / alpha builds and not stable? Evidently, that isn't as technically complex since it's been done, but it is a change from one type of build to another.

LeonarddeR commented 6 years ago

Ah, I think I should have pointed out that I really agree with your proposal here, I was just only viewing it from a technical perspective when I replied initially.

Furthermore, I realised that it isn't at all as complex as I thought. I think we could actually do something in the config spec that is based on the buildVersion.isTestVersion value. We are already using string formatting on the config spec for latestSchemaVersion, so it would be a change of a line or two to accomplish this. Note that this will set the default value, it won't have any effect as soon as one updates the log level manually.

Brian1Gaff commented 6 years ago

As a matter of interest, what would be the implications to having full logging all the time. Does it slow things down or something. I have to say I've not noticed it and I always have it set to debug. Also as I have said before, it might be interesting to have a setting on release versions to turn on error sounds, obviously normally off, so it can help users with odd issues to look at log files when reporting a problem, ie they would be aware when the error actually started.

Brian

bglists@blueyonder.co.uk Sent via blueyonder. Please address personal E-mail to:- briang1@blueyonder.co.uk, putting 'Brian Gaff' in the display name field.


DrSooom commented 6 years ago

If you change the default logging level to "Debug", you must inform the user about this on startup. Maybe the end user isn't familiar with the differences between the different release channels, so he could think that the logging level would be set to "Info" even on alpha and try builds. Otherwise that could end into a security and privacy issue.

Personally, the idea isn't bad. For testers only it makes a huge sense.

Questions:

  1. Is it possible to add the combobox for the logging level to the Welcome Dialog for alpha and try builds only?
  2. Is it possible to change the default logging level from "Info" to "Disable" to all release channels of NVDA? (You might already know why I prefer this. ;) )

If both is possible, unfamiliar users would be protected and advanced user and testers are able to change the logging level directly after the first start of NVDA on the Welcome Dialog. Furthermore you shouldn't forget that it is currently possible to set the logging level via a command line parameter. But I'm not sure, if this also works on the NVDA Installer (= temp copy of NVDA). I guess no, but I'm not sure.

DrSooom commented 6 years ago

I have another idea to solve this: Is it possible that the installer looks for a file called "alpha.log" (or so) in "C:\Users[Username]\AppData\Roaming\nvda"? If the file exists, the temp version of NVDA will be started automatically with the logging level set to "Debug", otherwise with the default logging level value. The file "alpha.log" itself can be empty here – an empty text file to be exactly.

What do you think about this?

josephsl commented 6 years ago

Hi, any program can create “alpha.txt”, or in some cases, one can delete it, fooling the installer into thinking that info level is sufficient. Thus I’m sort of hesitant regarding this one. Thanks.

From: Daniel Mayr notifications@github.com Sent: Friday, October 26, 2018 8:18 AM To: nvaccess/nvda nvda@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: Re: [nvaccess/nvda] Feature change: Set log level to debug on Alpha (& try build) snapshots (#8880)

I have another idea to solve this: Is it possible that the installer looks for a file called "alpha.log" (or so) in "C:\Users[Username]\AppData\Roaming\nvda"? If the file exists, the temp version of NVDA will be started automatically with the logging level set to "Debug", otherwise with the default logging level value. The file "alpha.log" itself can be empty here – an empty text file to be exactly.

What do you think about this?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nvaccess/nvda/issues/8880#issuecomment-433443879 , or mute the thread https://github.com/notifications/unsubscribe-auth/AHgLkP2335DZIi_MaYwOBiUpFfaQ249Gks5uoyeqgaJpZM4X5oWJ .

jscholes commented 6 years ago

This proposal makes me a bit uncomfortable. I can certainly see the benefit of having debug logging enabled by default for investigating staartup issues or problems that are difficult to reproduce reliably. On the other hand, the debug log is not context aware and thus will contain a lot of sensetive information if left enabled for the lifetime of the session. It in effect turns NVDA into a keylogger. It wouldn't be difficult for someone to write a piece of malware which could potentially scrape passwords, credit card numbers and whatever else from NVDA logs.

There are also minor disk space concerns, with NVDA writing 600KB in the last 20 minutes of normal usage here when debug logging is turned on. It's fairly certain that a number of users won't bother reading any new privacy notice that may be added either, and ignoring such a notice regarding debug logging by default could have severe consequences.

Perhaps, if the goal is to gain more visibility into startup issues, debug logging could be temporarily enabled at startup and then switched off once the core was successfully initialised? Or maybe this would work only for try builds, if an appropriate disclaimer was added advising that such builds should not be used to handle possibly sensetive information.

DrSooom commented 6 years ago

What do you think about a new DWORD value in the Windows Registry in "HKEY_LOCAL_MACHINE\SOFTWARE\nvda" and in "HKEY_LOCAL_MACHINE\SOFTWARE\WOW6432Node\nvda"? But I don't know if the installer can read information from the Windows Registry without admin privileges.

jscholes commented 6 years ago

It's reasonable to assume that a number of users run snapshot or try builds as portable copies, while keeping a stable version for their main installation. So if this does get implemented, touching the registry isn't appropriate. Nor should it be necessary; as @leonardder mentioned it should be possible to handle this in the config spec.

josephsl commented 6 years ago

Hi, same problem as file approach: someone can add or delete this reg key while the installer is running or not, either by human means or through registry API. In short, although it is important to think about human-centered approach, we should never forget that there might be hurdles when it comes to bringing a feature into reality, and be prepared to examine possible (if not all) scenarios (as in what could go wrong). Thanks.

From: Daniel Mayr notifications@github.com Sent: Friday, October 26, 2018 8:52 AM To: nvaccess/nvda nvda@noreply.github.com Cc: Joseph Lee joseph.lee22590@gmail.com; Comment comment@noreply.github.com Subject: Re: [nvaccess/nvda] Feature change: Set log level to debug on Alpha (& try build) snapshots (#8880)

What do you think about a new DWORD value in the Windows Registry in "HKEY_LOCAL_MACHINE\SOFTWARE\nvda" and in "HKEY_LOCAL_MACHINE\SOFTWARE\WOW6432Node\nvda"? But I don't know if the installer can read information from the Windows Registry without admin privileges.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/nvaccess/nvda/issues/8880#issuecomment-433454867 , or mute the thread https://github.com/notifications/unsubscribe-auth/AHgLkIy9mZWH39joXxKIZ1coYmyv33PFks5uoy-KgaJpZM4X5oWJ .

tspivey commented 6 years ago

I agree with @jscholes, and would prefer not to have my debug logs written to disk all the time when running a snapshot. To solve this, I propose the following: Keep debug messages in memory for x amount of time, say 30 seconds to a minute, and provide a way of accessing them. This would be in addition to whatever logging level the user has set.

DrSooom commented 6 years ago

@jscholes: You are right. My failure.

Is it possible to start the Installer with a parameter which will change the logging level via an additional parameter for the nvda.exe, which must be copied to the temp folder first by the installer itself)?

DrSooom commented 6 years ago

See also issue #8515.

DrSooom commented 6 years ago

After thinking on this, I now see no significant reasons, why we should change the default logging level for alpha and try builds. Because it is currently possible for installed and portable version of NVDA to start the "nvda.exe" with the command parameter "-l=10". Furthermore testers already should know how to change the logging level via the GUI or directly in the "nvda.ini" using an source code editor.

So in the end we only need a solution to set the logging level to 10 via the Installer for the startup procedure of the temporary version of NVDA. After the temporary version of NVDA started correctly, you are already able to change the logging level via the GUI.

So I'm still asking me if we could add a parameter to the NSIS Installer which will set the logging level of "nvda.exe" to "10". That would be the securest way in my opinion.

We can also implement a new parameter called "startup-logging" which will set the logging level to 10 and creates the "nvda.log" file directly on the desktop, but with the different that it stops (= set logging level to "100" = OFF) automatically after x seconds. the amount of seconds can be set as the value for the "startup-logging" parameter. Minimum = 1, maximum = 300 and default = 30.

Any thoughts about this?

josephsl commented 3 months ago

Hi,

Almost six years later...

Would typing "path/to_version.exe --debug-logging" from command-line suffice? If yes, I think this should be promoted as a possible solution to this issue.

Thanks.

seanbudd commented 3 months ago

I think it's worthwhile having debug as the default log level for source, PR and try build snapshots. The CLI option has been available for a long time, but this is about the convenience factor.

I'm less certain about alpha builds, debug logging is a minor security risk and changing this default for "daily driver" builds might be risky.

SaschaCowley commented 1 month ago

Here to second debug logging by default when running from source at the very least. PR and try builds are probably a good idea as well. I agree that enabling debug logging by default for alpha is a security risk and probably a step too far.