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

RFC: add logger that logs into browser console #4702

Merged
merged 8 commits into from
Apr 13, 2018

Conversation

betodealmeida
Copy link
Member

This PR integrates the console_log module with Superset, allowing records to be logged directly into the browser console in realtime. It adds a new option to the CLI, --console-log, which enables debug mode and creates a new root logger called "console".

Demo

I temporarily added the following logging code:

diff --git a/superset/viz.py b/superset/viz.py
index 0c552623..a2b3d508 100644
--- a/superset/viz.py
+++ b/superset/viz.py
@@ -2044,6 +2044,10 @@ class DeckScatterViz(BaseDeckGLViz):
         }

     def get_data(self, df):
+        console = logging.getLogger('console')
+        console.info(df)
+        console.info(self.form_data)
+
         fd = self.form_data
         self.point_radius_fixed = fd.get('point_radius_fixed')
         self.fixed_value = None

console_log

Note how the Pandas data frame is logged as if it were printed in Python (using the repr function), and the form data is logged as a Javascript object, allowing it to be inspected.

Notes

  1. When logging to console, the Flask app is run using a gevent server instead of the standard Flask server used in debug mode. This is because the websocket created by console_log is based on gevent.
  2. The gevent server does not have a reloader, but we can leverage the one in Werkzeug. Doing this requires calling gevent.monkey.patch_all(), in order to make the Werkzeug code event based. Monkey patching is considered a bad practice in general, though gevent is pretty stable and its patching is pretty safe.
  3. This can also be used in production with gunicorn, by passing the argument -k "geventwebsocket.gunicorn.workers.GeventWebSocketWorker". In this PR the logging is only enabled in debug mode, nevertheless, since it's very experimental.

I'm publishing this as an RFC since I understand these points might be controversial.



def console_log_run(app, port, use_reloader):
from console_log import ConsoleLog
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put these imports at the top

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know imports should be top-level, but these are optional requirements (see the modified setup.py). If they were in the top-level the CLI we would force everybody to install console_log (and its dependencies: gevent, gevent-websocket, etc.).

@hughhhh
Copy link
Member

hughhhh commented Mar 28, 2018

🚢

@codecov-io
Copy link

codecov-io commented Mar 28, 2018

Codecov Report

Merging #4702 into master will decrease coverage by 0.06%.
The diff coverage is 29.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4702      +/-   ##
==========================================
- Coverage   72.39%   72.33%   -0.07%     
==========================================
  Files         208      208              
  Lines       15548    15568      +20     
  Branches     1204     1204              
==========================================
+ Hits        11256    11261       +5     
- Misses       4289     4304      +15     
  Partials        3        3
Impacted Files Coverage Δ
superset/views/core.py 74.64% <100%> (ø) ⬆️
superset/__init__.py 72.32% <100%> (+0.24%) ⬆️
superset/cli.py 44.06% <19.04%> (-2.77%) ⬇️

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 2f5cff7...21f6fec. Read the comment docs.

@mistercrunch
Copy link
Member

LGTM but it could use a description in CONTRIBUTING.md !


from superset import app
app.logger.error('An exception occurred!')
app.logger.info(form_data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh does logging.debug('Foo') work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me give it a try.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mistercrunch, currently it won't work because console_log adds the websocket handler to a single logger instance ­— Flask uses it's own (flask.app), and it's different from the root logger. I'll modify the library so that it takes more than one logger instance, should be straightforward.

@@ -745,8 +745,10 @@ def shortner(self):
obj = models.Url(url=url)
db.session.add(obj)
db.session.commit()
return('http://{request.headers[Host]}/{directory}?r={obj.id}'.format(
request=request, directory=directory, obj=obj))
return Response(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious on the angle here, you prefer an explicit Response object?

@mistercrunch
Copy link
Member

LGTM

@mistercrunch
Copy link
Member

Merge conflicts :(

@mistercrunch mistercrunch merged commit fd84fd8 into apache:master Apr 13, 2018
@mistercrunch mistercrunch deleted the DPTOOLS-392_surface_granularity branch April 13, 2018 04:48
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
* Option for logging into browser console

* Move import

* Add lint req

* Add docs, use Flask logger
timifasubaa pushed a commit to timifasubaa/incubator-superset that referenced this pull request May 31, 2018
* Option for logging into browser console

* Move import

* Add lint req

* Add docs, use Flask logger
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* Option for logging into browser console

* Move import

* Add lint req

* Add docs, use Flask logger
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.25.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.25.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants