Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Logging for v1.35 #114

Merged
merged 23 commits into from
Apr 15, 2021
Merged

Logging for v1.35 #114

merged 23 commits into from
Apr 15, 2021

Conversation

jeffsf
Copy link
Contributor

@jeffsf jeffsf commented Apr 9, 2021

The availability of logs during the lead-up to v1.35 has been very valuable to identify issues, be they with the code, the directions, the operator, or even the machine.

These commits assign priorities to hundreds of the previous msg command to make them "filterable" both for developers as well as for users and support-team members, properly provide 64 kB buffering to respect the concerns around flash wear, and enable logging for all users. Stable users will run "INFO" by default, with "DEBUG" available through a plug-in (or future skin modifications). Users on pre-Stable branches will log at the "DEBUG" level.

The "default" settings page has been modified to reflect that logging is on. This will help reduce confusion from experienced users expecting to see an option there, as well as less-experienced ones following outdated instructions.

These are "interim" severity assignments.

The eventual objective is that at the NOTICE level one gets a "quick sketch" of the operation of the machine. At the INFO level, there are enough details to determine, for example, when the scale tare was done, without overwhelming a non-technical users.

There's still a lot of "doubling" with both de1_comms and bluetorh reporting the same event. My plan in the future is to be able to report at the higher level (de1_comms) with a moderate log level and the lower level at a lower level. Users at the INFO level would then only see one of the pair of log messages.

Making the messages "denser" in content and eliminating non-printable "binary" seems needed to be able to achieve these objectives, as well as making it easier to select the "important enough" logging points for the NOTICE level and those that should be pushed down to the DEBUG level.

Thanks to @Mimoja and #90 for the contributions.

Many attempts to set bluetooth devices are done before the device is known to be connected. These, contrary to my instincts demanding checking before they are called, are presently logged as DEBUG, though they arguably should be logged as a WARNING or ERROR in logic that let the call be made at all. Resolving that would require a rework of bluetooth.tcl.

No significant work around changing the content of the messages is in this PR for v1.35. There has been significant work past this to reduce the size of the messages, eliminate "binary" data, and to make the messages more meaningful for a non-expert in that section of code. As these sometimes involve processing of the messages, especially within bluetooth.tcl, changes of this type are planned to be deferred to after the v1.35 release.

@jeffsf jeffsf changed the title Logging for v135 Logging for v1.35 Apr 10, 2021
@decentjohn
Copy link
Contributor

This is all great stuff. Can I ask you that you copy this documentation stuff in the manual, at a chapter/section where you feel it's appropriate? Otherwise it'll get lost here.

That goes for a lot of the other big sets of documentation you've written.

Feel free to create a page as needed, and I'll edit the table of contents to include a ref to it.

@jeffsf
Copy link
Contributor Author

jeffsf commented Apr 10, 2021

Change in commit message 4941b57 to reflect that the name of the method is ::logging::flush_log

@jeffsf
Copy link
Contributor Author

jeffsf commented Apr 13, 2021

Rebased due changes on main

Added severity to binary, machine, plugins, updater, utils, vars

Replaced and relocated previously non-functional close_log_file calls

Updated log_debug plugin

Based on discussion with @decentjohn , removed obsolete check_for_multiple_listbox_events_bug

Local work on de1_comms and bluetooth continues. Though seemingly solid here, that these now call other procs for formatting the binary data makes them too risky for inclusion in an imminent v1.35

@jeffsf
Copy link
Contributor Author

jeffsf commented Apr 13, 2021

Before

de1plus]$ egrep 'msg [^-]' *.tcl | cut -d ":" -f 1 | wc -l
     559
de1plus]$ egrep 'puts ' *.tcl | cut -d ":" -f 1 | wc -l
     233

de1plus]$ egrep 'msg [^-]' *.tcl | cut -d ":" -f 1 | sort | uniq -c
   1 autopair_with_de1.tcl
   8 binary.tcl
 195 bluetooth.tcl
 136 de1_comms.tcl
   2 event.tcl
  55 gui.tcl
   1 logging.tcl
  29 machine.tcl
   6 plugins.tcl
   1 profile.tcl
  22 updater.tcl
  39 utils.tcl
  64 vars.tcl

de1plus]$ egrep 'puts ' *.tcl | cut -d ":" -f 1 | sort | uniq -c
   2 autopair_with_de1.tcl
  11 binary.tcl
   1 de1_comms.tcl
  67 gui.tcl
   2 logging.tcl
  11 misc.tcl
  25 updater.tcl
  64 utils.tcl
  50 vars.tcl

After
(most are due to use of "msg" as a variable or files like misc.tcl that haven't been reviewed):

de1plus]$ egrep 'msg [^-]' *.tcl | cut -d ":" -f 1 | sort | uniq -c
   1 autopair_with_de1.tcl
   1 bluetooth.tcl
   1 de1_comms.tcl
   2 event.tcl
   7 gui.tcl
   1 logging.tcl
   1 profile.tcl
   3 vars.tcl

de1plus]$ egrep 'puts ' *.tcl | cut -d ":" -f 1 | sort | uniq -c
   2 autopair_with_de1.tcl
   2 logging.tcl
  11 misc.tcl
   2 updater.tcl
   5 utils.tcl

This was referenced Apr 14, 2021
@jeffsf
Copy link
Contributor Author

jeffsf commented Apr 15, 2021

Resolved Tk rendering of an inactive, yet selected checkbox in commit 41d00ec

image

Modified log_uploader plugin to ignore the "placebo" log_enabled setting in commit bdbdab0 (squashed a commit around using::logging::flush_log as that is part of the PR, so can be included without dependency on another PR)

jeffsf added 22 commits April 15, 2021 11:10
Previous log-rotation code was hard-wired to "log.txt"

Adjust to use the value of $::settings(logfile) as set
in code executed just prior to log rotation.

Signed-Off-By: Jeff Kletsky <[email protected]>
Logs to the Android logs through AndroWish `borg log` function.

These logs are not delayed by file buffering and are suitable
for real-time monitoring.

Logs can be read with, for example:

adb logcat DE1:V BLEClient:V *:S

More details on using adb and logcat at
https://developer.android.com/studio/command-line/logcat

NB: Default Android logging level for "production" builds is Info
    One way to alter this is thrugh adb shell:

    em_t8880_64mme_jinsen:/ # setprop log.tag.DE1 V
    em_t8880_64mme_jinsen:/ # setprop log.tag.BLECLient V

As these Android properties are not persisted through a boot,
developers may find defining a local alias or shell script helpful.
One example is:

alias de1app_logcat='adb shell setprop log.tag.DE1 V ; \
adb shell setprop log.tag.BLECLient V ; \
adb logcat DE1:V BLEClient:V *:S'

which will monitor V[erbose] messages from the DE1 and `ble`
from AndroWish (ble/src/tk/tcl/wish/BLEClient.java), while
S[ilencing] other messages.

"catch" as ::android isn't defined until later in the load cycle

Signed-Off-By: Jeff Kletsky <[email protected]>
Due to flash-wear concerns, use 64 kB buffering.

Android logging is provided with an earlier commit to meet real-time,
log-monitoring needs.

Although commit 2a45204 adjusted the buffer size in the former logical
branch on $::settings(log_fast), the earlier commit f433940 added
`set ::settings(log_fast) 1` rendering that branch unreachable, as well as
locking all users into "log_fast" due to the persistance of ::settings().

Deprecated ::settings(log_fast)

DEVELOPER NOTES
===============

Plugins and skins that access the log files directly
should flush prior to actions such as uploading.
::logging::flush_log {} is provided as a convenience.

Signed-Off-By: Jeff Kletsky <[email protected]>
Preserve the log contents in case there is an unclean exit from the app
or for users that sync the log content to other machines.

Signed-Off-By: Jeff Kletsky <[email protected]>
Some skins supported on-screen viewing of the logs in real time
when in "developer" modes. With the availability of real-time
logging with logcat, remove this feature to reduce maintainance.

References to ::debuglog will be removed in a subsequent commit.

Signed-Off-By: Jeff Kletsky <[email protected]>
Removed by a prior commit, ::debuglog was rendered in some skins
by modifyig the GUI layout. This code appears to have been copied
from Insight into several skins:

  skins/Insight/skin.tcl
  skins/SWDark3/skin.tcl
  skins/SWDark4/skin.tcl

References in metric and DSx will need to be modified upstream.

Signed-Off-By: Jeff Kletsky <[email protected]>
Stable builds generally should not require DEBUG-level logging with
the default set to INFO. At INFO, omce the severity has been set on
the majority of `msg` calls, normal operation should be able to be
confirmed by examining the logs, reducing the load on support.

For the cases where abnormalities appear on Stable builds, the logs
can be increased to DEBUG level by enabling the log_debug plugin.
Although this does not cover the period between app start and the
load and start of plugins, it should be sufficient to provide
visibility into operational issues.

Logging to the console and Android logging are not impacted.

Signed-Off-By: Jeff Kletsky <[email protected]>
comms_msg is gated by ::settings(comms_debugging) and was previously
a single-argument proc. As a result, it could not handle severity flags.

Now calls msg {*}$args

Signed-Off-By: Jeff Kletsky <[email protected]>
Retained only the last of the six repeated definitions

Signed-Off-By: Jeff Kletsky <[email protected]>
First pass at assigning reasonable severituy codes for msg in

  bluetooth.tcl
  de1_comms.tcl
  gui.tcl
  utils.tcl

Commented-out msg lines removed

Converted one "gated" comms_msg to msg

No work done to ensure that no binary data is being logged.

Logging of add_de1_image removed as per discussion with John
load_font completion retained at DEBUG level at this time

These severities are likely to change in future commits,
but need to get at least a first-cut at usability.

Efforts of Mimoja are included in this commit, see
#90

Signed-Off-By: Jeff Kletsky <[email protected]>
First pass at assigning reasonable severituy codes for msg in

  bluetooth.tcl
  de1_comms.tcl
  gui.tcl
  utils.tcl

Commented-out lines removed

No work done to ensure that no binary data is being logged.

    proc append_file {filename data} had a puts for everything
    appended.  As this can be binary, it was not converted and
    commented out.

    proc render_arabic {args} potentially dumps the input string on
    error which may not be able to be rendered in the logs.
    Retained at this time.

These severities are likely to change in future commits,
but need to get at least a first-cut at usability.

DEVELOPER NOTES
===============

Errors in calling add_de1_widget will now be logged rather than
only appearing on the console.

The log message has not been updated to include the options dict
that could be returned by the catch {} statement around that call.

Signed-Off-By: Jeff Kletsky <[email protected]>
To facilitate log filtering, either post facto or within the app,
ensure each line has a header.

These headers all have the same timestamp.

Signed-Off-By: Jeff Kletsky <[email protected]>
Remove identified uses of [stacktrace] except in cases where
an unexpected  condition has occurred that requires knowing
the call stack to identify and resolve.

Signed-Off-By: Jeff Kletsky <[email protected]>
With logging now repsecting concerns for flash wear, providing better
severity codes, and, by default, removing DEBUG messages from Stable,
enable for all users.

DEVELOPER NOTES
===============

::settings(log_enabled) is now unsued.

It is retained to prevent skins from causing errors.

Skins may remove the logging on/off control.

If a control to allow the user to select logging level is provided,
please ensure that the choices are only for Stable and that tho
preferred limit of options at this time is DEBUG and INFO (default).
Beta and Nightly users should be logging at DEBUG at all times.

Signed-Off-By: Jeff Kletsky <[email protected]>
Previous commits enabled logging for all users and ignored ::settings(log_enabled)

This commit changes the checkbox on the "default" Settings > Misc
screen to disabled and to read "Logging is enabled".

This choice is intended to provide messaging at the point where a user,
following older instructions, might try to enable logging.

In the future, skins may wish to provide an option to Stable-branch
users between DEBUG and INFO levels. At this time, the Log DEBUG
plugin can be activated, when a Stable-branch user or the support team
needs to examine logs at the DEBUG level.

Signed-Off-By: Jeff Kletsky <[email protected]>
Old language assumed Skale or Decent, even if another make was found.

Removed specific language. Message includes type found.

Signed-Off-By: Jeff Kletsky <[email protected]>
First pass at assigning reasonable severituy codes for msg in

    binary.tcl
    machine.tcl
    plugins.tcl
    updater.tcl
    utils.tcl
    vars.tcl

Resolves a potential silent failure after an HTTP GET

Commented-out msg lines removed

No work done to ensure that no binary data is being logged.

These severities are likely to change in future commits,
but need to get at least a first-cut at usability.

Signed-Off-By: Jeff Kletsky <[email protected]>
Although the interpreter should close open files on exit,
restore the intent of close_log_file on exit.

Signed-Off-By: Jeff Kletsky <[email protected]>
Obsoleted by unification into ::logging

Signed-Off-By: Jeff Kletsky <[email protected]>
Unified with msg in ::logging, references no longer exist within updater.tcl

Signed-Off-By: Jeff Kletsky <[email protected]>
Removed now-unused proc preload

Signed-Off-By: Jeff Kletsky <[email protected]>
Formerly in vars.tcl, call and message previously commented out.

As per John, "looks like I fixed that bug"

Signed-Off-By: Jeff Kletsky <[email protected]>
As logging is enabled on v1.35 at all times, replace the previous check
for ::settings(log_enabled) with a readability check on the log file.

Rather than flushing the log explicitly by filehandle,
utilize the utility ::logging::flush_log to provide
independence from underlying implementation.

Signed-Off-By: Jeff Kletsky <[email protected]>
@jeffsf
Copy link
Contributor Author

jeffsf commented Apr 15, 2021

Rebased on current upstream HEAD; commit dac1635 (upstream/main, upstream/HEAD)

@Mimoja Mimoja merged commit 9d2a001 into decentespresso:main Apr 15, 2021
@jeffsf jeffsf deleted the pr-2021-04-09-logging-for-v135 branch April 15, 2021 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants