Logging/emitters refactoring by fjarri · Pull Request #1103 · nucypher/nucypher

fjarri · 2019-06-26T04:46:48Z

A reworked PR #1079, branched off the current croquetas, since there were a lot of changes introduced there.

Changes:

All options are now specified after commands. That is, if before one would call
```
  $ nucypher alice --debug init --federated-only
```
after the PR the syntax would be
```
  $ nucypher alice init --debug --federated-only
```
Pros:
- helps avoid code duplication (for option definition and processing).
- helps user discover these options in --help. That is, if before, in order to find out about --debug, one would have to call nucypher --help in addition to nucypher alice --help, now just nucypher alice --help will be enough - all the options will be there.
A console output emitter is created only once (in NucypherClickConfig) and propagated to all classes/functions that need it.
All click.echo() and click.secho() calls are replaced with emitter.echo() calls, so that it can control that output too.
Instead of dispatching by keyword arguments in emitter(), specialized methods are used (banner(), message(), ipc(), echo()).
Output trapping in emitters was removed, since it did not seem to be used anywhere.
Added --log-level option from PR Fixes debug docker problem. #1095
--verbosity and --quiet now govern console output only (not console observer for logs). The output shown with --quiet on should remain the same as before, --verbosity currently only sets the state of the emitter - actually using it is a matter for another PR. Probably enough to close issue Implement CLI Verbosity Levels and Toggle #1018
NOTE: before, when --quiet was on, logs were written to text file, but not to JSON file. Not sure if it was an omission or a conscious design decision. Now both JSON and text logs are turned on or off together, it can be rolled back.
Parsing boolean envvars in NucypherClickConfig more rigorously using strtobool().
Added specific options --console-logs/--no-console-logs, --file-logs/--no-file-logs and --sentry-logs/--no-sentry-logs to override environment variables without resorting to --debug. Also made option description slightly more detailed.

TODO (may be better to leave for another PR):

Add --no-fun for turning off banners?
Instead of specifying color in echo() and message(), use only semantic markup and leave colorizing to the emitter.
Go through emitter.message() and emitter.echo() calls and decide which verbosity levels they should have.

To decide (probably for another PR too):

What to do with a few remaining click.secho() and print() which lie beyond the scope of NucypherClickConfig
characters/control/controllers.py still creates independent emitters; need to figure out if it is really necessary, and if not, how to propagate the main emitter there

codecov · 2019-06-26T04:58:59Z

Codecov Report

Merging #1103 into sekanjabin will increase coverage by 1.38%.
The diff coverage is 76.29%.

@@              Coverage Diff               @@
##           sekanjabin    #1103      +/-   ##
==============================================
+ Coverage          82%   83.39%   +1.38%     
==============================================
  Files              72       71       -1     
  Lines            9733     9704      -29     
==============================================
+ Hits             7982     8093     +111     
+ Misses           1751     1611     -140

Impacted Files	Coverage Δ
nucypher/characters/chaotic.py	`70.24% <ø> (+0.13%)`	⬆️
nucypher/cli/characters/moe.py	`64% <0%> (-2.67%)`	⬇️
nucypher/cli/main.py	`100% <100%> (+11.29%)`	⬆️
nucypher/characters/control/interfaces.py	`96.26% <100%> (ø)`	⬆️
nucypher/cli/characters/enrico.py	`71.05% <100%> (+1.82%)`	⬆️
nucypher/cli/characters/bob.py	`68.83% <50%> (-0.4%)`	⬇️
nucypher/cli/status.py	`78.57% <50%> (ø)`	⬆️
nucypher/cli/stake.py	`70.96% <55%> (-0.56%)`	⬇️
nucypher/cli/characters/felix.py	`69.56% <56.25%> (-1.09%)`	⬇️
nucypher/characters/control/controllers.py	`88% <57.14%> (ø)`	⬆️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df10a43...b40edc4. Read the comment docs.

fjarri · 2019-06-26T05:09:25Z

(Sorry, forgot to set it as draft, please consider it to be one)

derekpierre

Good refactor. I really like the emitter calls, they are much cleaner. Some comments mostly around the use of emitter.error.

I guess one catch was that we have to pass emitters to some functions. Is there a way around this? Even if not, I'm still pro this PR.

derekpierre · 2019-07-22T17:17:18Z

nucypher/cli/actions.py

+def select_client_account(blockchain, prompt: str = None, default=0) -> str:
+    enumerated_accounts = dict(enumerate(blockchain.client.accounts))
+    for index, account in enumerated_accounts.items():
+        click.secho(f"{index} | {account}")


Presumably, this line and similar lines in the other methods below should be using the emitter instead? I guess this change was part of a merge, after your changes...

These functions are called outside of the scope of NucypherClickConfig, so I'm not sure what to do with them. I suppose an emitter can be created in deploy() and passed to them.

derekpierre · 2019-07-22T17:25:11Z

nucypher/cli/characters/felix.py

                raise
            else:
-                click.secho(str(e), fg='red', bold=True)
+                emitter.echo(str(e), color='red', bold=True)


Is the reason we don't want to simply log using emitter.error because of verbosity and color? There is an associated comment for the emitter.error method to allow colour to be specified as a parameter, would verbosity make sense as well? - default verbosity would be 1, but could be specified otherwise.

I didn't want to overcomplicate the PR and only limit it to global changes. The next one would eliminate explicit color/boldness specification in emitter calls and use some semantic marking instead.

derekpierre · 2019-07-22T17:26:41Z

nucypher/characters/control/emitters.py


-        else:
-            raise ValueError('Either "response" dict or "message" str is required, but got neither.')
+    def error(self, e):


Should we allow colour to be specified as a parameter for error logging? - red can be default. Should bold=True also be the default.

I would prefer to avoid that, see above (and TODO in the PR description). I would rather eliminate all explicit style specification from the code, since it is hard to synchronize it between people, and you can never be sure whether to use red, or green, or some other color in some specific case - semantic calls like error() or warning() are more clear.

derekpierre · 2019-07-22T17:30:00Z

nucypher/cli/characters/felix.py

            raise
        else:
-            click.secho(str(e), fg='red', bold=True)
+            emitter.echo(str(e), color='red', bold=True)


Same comment about emitter.error as above.

derekpierre · 2019-07-22T17:33:13Z

nucypher/cli/characters/ursula.py

                raise
            else:
-                click.secho(str(e), fg='red', bold=True)
+                emitter.echo(str(e), color='red', bold=True)


Use emitter.error?

My plan for this PR was to just switch to explicit emitter usage without changing any related calls (to make sure I'm not changing the logging behavior - e.g. error() would log the error, while echo() doesn't). We can discuss any changes to logging strategies later when we get the blocking changes out of the way.

derekpierre · 2019-07-22T17:39:32Z

nucypher/characters/control/emitters.py

-            raise ValueError('Either "response" dict or "message" str is required, but got neither.')
+    def error(self, e):
+        if self.verbosity >= 1:
+            e_str = str(e)


Would it make sense to add the class name of the error as well? i.e. e.__class__.__name__. Without it we are hoping that the message alone provides sufficient context.

As above, this PR intends to change the emitter API while preserving the current behavior (because I'm still not sure what kind of logic is needed in different cases).

derekpierre · 2019-07-22T17:47:50Z

nucypher/cli/deploy.py

+    try:
+        blockchain.connect(fetch_registry=False, sync_now=sync)
+    except BlockchainDeployerInterface.ConnectionFailed as e:
+        emitter.echo(str(e), color='red', bold=True)


emitter.error?

michwill

Looks good

…ties/logging Also add a check that Sentry SDK is only initialized once

…ng to NucypherClickConfig

There's was not much happening between the beginning of start() and echo(), so moving the status printing should not obscure anything.

…gate it everywhere

…and add specific options for each type of logging Now quiet/verbose govern console output (emitters)

…r CLI.

fjarri added Enhancement New or improved features CLI This effects the nucypher CLI Logging Related to console or file logging labels Jun 26, 2019

fjarri mentioned this pull request Jun 26, 2019

[WIP] CLI options refactoring #1079

Closed

fjarri requested a review from KPrasch June 26, 2019 04:47

fjarri changed the title ~~Logging/emitters refactoring~~ [WIP] Logging/emitters refactoring Jun 26, 2019

fjarri requested a review from jMyles June 26, 2019 05:53

fjarri changed the title ~~[WIP] Logging/emitters refactoring~~ Logging/emitters refactoring Jun 27, 2019

tuxxy force-pushed the mimosa branch from a853dfd to a21b01d Compare July 11, 2019 21:39

fjarri force-pushed the output-refactoring branch from 952086a to ebb5840 Compare July 17, 2019 22:03

fjarri changed the base branch from mimosa to master July 17, 2019 22:04

fjarri force-pushed the output-refactoring branch from ebb5840 to ee12071 Compare July 17, 2019 22:09

fjarri changed the base branch from master to sekanjabin July 17, 2019 22:14

fjarri force-pushed the output-refactoring branch from 966757a to 289c716 Compare July 18, 2019 04:51

fjarri assigned vepkenez, cygnusv, tuxxy and derekpierre and unassigned vepkenez, cygnusv, tuxxy and derekpierre Jul 18, 2019

fjarri requested review from cygnusv, derekpierre, tuxxy and vepkenez July 18, 2019 04:55

KPrasch mentioned this pull request Jul 18, 2019

[EPIC] Sekanjabin - StakeHolder & Worker-Ursula Integration, Deployer Coverage, UX Baseline. #1124

Merged

13 tasks

derekpierre reviewed Jul 22, 2019

View reviewed changes

fjarri force-pushed the output-refactoring branch 2 times, most recently from b40edc4 to 8a16b37 Compare July 23, 2019 05:54

KPrasch mentioned this pull request Jul 24, 2019

fixed emitter error for bob #1132

Closed

michwill approved these changes Jul 25, 2019

View reviewed changes

fjarri added 7 commits July 25, 2019 09:14

Hide registering/unregistering observers and Sentry init inside utili…

fd0bf05

…ties/logging Also add a check that Sentry SDK is only initialized once

Remove duplicate options, move shared options definition and processi…

eb641eb

…ng to NucypherClickConfig

Move echo() calls one level higher and replace them with emit()

2ae2a73

There's was not much happening between the beginning of start() and echo(), so moving the status printing should not obscure anything.

Create a console emitter only once (in NucypherClickConfig) and propa…

e63c7f8

…gate it everywhere

Use specialized methods in WebEmitter as well

5e0abff

More strict boolean envvars parsing

758a5c3

Rework verbose/quiet options, add more detailed option descriptions, …

f2a96bc

…and add specific options for each type of logging Now quiet/verbose govern console output (emitters)

KPrasch force-pushed the output-refactoring branch from 8a16b37 to cc93cce Compare July 25, 2019 16:32

Ensure CLI painting implements emitters; Emitter logic for stakeholde…

84e1f49

…r CLI.

KPrasch force-pushed the output-refactoring branch from cc93cce to 84e1f49 Compare July 25, 2019 17:04

KPrasch approved these changes Jul 25, 2019

View reviewed changes

derekpierre approved these changes Jul 25, 2019

View reviewed changes

KPrasch merged commit 7da9cde into nucypher:sekanjabin Jul 25, 2019

fjarri deleted the output-refactoring branch July 27, 2019 02:37

Conversation

fjarri commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fjarri commented Jun 26, 2019

Uh oh!

derekpierre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michwill left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fjarri commented Jun 26, 2019 •

edited

Loading

codecov bot commented Jun 26, 2019 •

edited

Loading