Skip to content

feat: refactor on DBEventLogger to allow for context management#13441

Merged
hughhhh merged 22 commits intomasterfrom
hugh/event-logger-refactor
Mar 5, 2021
Merged

feat: refactor on DBEventLogger to allow for context management#13441
hughhhh merged 22 commits intomasterfrom
hugh/event-logger-refactor

Conversation

@hughhhh
Copy link
Copy Markdown
Member

@hughhhh hughhhh commented Mar 3, 2021

SUMMARY

Inspired the discussion in #13346 to allow us to log events with context.

with event_logger(action='foo'):
	do_something()

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 3, 2021

Codecov Report

Merging #13441 (f483764) into master (9fc03f0) will increase coverage by 0.16%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13441      +/-   ##
==========================================
+ Coverage   77.44%   77.61%   +0.16%     
==========================================
  Files         906      906              
  Lines       45721    46440     +719     
  Branches     5449     5524      +75     
==========================================
+ Hits        35410    36043     +633     
- Misses      10184    10270      +86     
  Partials      127      127              
Flag Coverage Δ
cypress 57.94% <ø> (+0.04%) ⬆️
hive 80.04% <100.00%> (+0.01%) ⬆️
javascript 63.40% <ø> (ø)
mysql 80.37% <100.00%> (+0.01%) ⬆️
postgres 80.41% <100.00%> (+0.01%) ⬆️
presto 80.27% <100.00%> (+0.21%) ⬆️
python 81.09% <100.00%> (+0.19%) ⬆️
sqlite 80.03% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/utils/log.py 93.98% <100.00%> (+1.00%) ⬆️
superset/app.py 79.77% <0.00%> (-1.73%) ⬇️
...set-frontend/src/dashboard/util/getDropPosition.js 90.90% <0.00%> (-1.52%) ⬇️
superset/dashboards/api.py 87.16% <0.00%> (-0.39%) ⬇️
superset/tasks/slack_util.py 89.65% <0.00%> (+0.18%) ⬆️
superset/config.py 91.40% <0.00%> (+0.65%) ⬆️
superset/dashboards/schemas.py 100.00% <0.00%> (+1.12%) ⬆️
superset/views/alerts.py 82.78% <0.00%> (+1.45%) ⬆️
superset/models/dashboard.py 79.38% <0.00%> (+2.60%) ⬆️
superset/dashboards/dao.py 97.91% <0.00%> (+2.67%) ⬆️
... and 2 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 9fc03f0...f483764. Read the comment docs.

@pull-request-size pull-request-size bot added size/L and removed size/S labels Mar 3, 2021
*args: Any,
**kwargs: Any,
):
self.records.append(kwargs)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You might want to store more stuff here, to check that user_id is set, for example.

Another cool thing you might want to do is test that duration is computed correctly. There's a nice Python module called freezegun that allows you to do that. Something like this:

from freezegun import freeze_time

# the first time datetime.now() is called returns 2020-01-14; subsequent calls
# are incremented by 15 seconds
@freeze_time("Jan 14th, 2020", auto_tick_seconds=15)
def test_context_manager_log(self):
    ...
    # change `log()` to also store duration
    assert logger.records[0]["duration"] == timedelta(seconds=15)

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
Copy link
Copy Markdown
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

double checking that this isn't a breaking change, otherwise this looks like it might solve something that we were doing with a custom logger class up until now

@hughhhh hughhhh marked this pull request as ready for review March 4, 2021 18:32
@hughhhh
Copy link
Copy Markdown
Member Author

hughhhh commented Mar 4, 2021

@etr2460 it won't be a breaking change since we mostly just moved some of the logic to log_context which we are leveraging for context management

Copy link
Copy Markdown
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

It looks great, I love that test! I just left a few comments on improving log_with_context.

hughhhh and others added 2 commits March 4, 2021 13:47
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
Copy link
Copy Markdown
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

You're awesome!

@hughhhh hughhhh changed the title chore: refactor on DBEventLogger to allow for context management feat: refactor on DBEventLogger to allow for context management Mar 5, 2021
@hughhhh hughhhh merged commit b17e7aa into master Mar 5, 2021
@hughhhh hughhhh deleted the hugh/event-logger-refactor branch March 5, 2021 20:12
allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021
…he#13441)

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 First shipped in 1.2.0 labels Mar 12, 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 size/L 🚢 1.2.0 First shipped in 1.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants