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

refactor: optimize backend log payload #11927

Merged
merged 2 commits into from
Dec 16, 2020
Merged

Conversation

ktmud
Copy link
Member

@ktmud ktmud commented Dec 4, 2020

SUMMARY

Follow up with #11920 and #11714 , implement a more robust event_logger payload.

  1. Replace path_no_int with request.url_rule, the match pattern used in Flask router
  2. Make sure JSON payload works for REST API requests
  3. Make sure to collect rison objects when present, and remove the raw query string.
  4. Use ClassName.method as action for REST API and add __qualname__ to object_ref for legacy actions.
  5. Accept functions as action and object_ref in the decorators
  6. Rename decorator log_manually to log_this_with_extra_payload

The goal is to make future analytic queries on the logs table as easy as possible and reduce redundancy.

cc @mistercrunch @etr2460 @john-bodley @robdiciuccio @dpgaspar @graceguo-supercat

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

image

After:

Snip20201204_46

TEST PLAN

Will add unit tests soon

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

@ktmud ktmud changed the base branch from master to erik-ritter--remove-logs-columns December 4, 2020 10:16
@ktmud ktmud force-pushed the remove--path-no-int branch 4 times, most recently from 18c4b85 to 60dd1ee Compare December 4, 2020 10:21
@pull-request-size pull-request-size bot added size/M and removed size/L labels Dec 4, 2020
@codecov-io
Copy link

codecov-io commented Dec 4, 2020

Codecov Report

Merging #11927 (5ca1d1f) into master (77cae64) will decrease coverage by 3.92%.
The diff coverage is 96.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11927      +/-   ##
==========================================
- Coverage   67.55%   63.62%   -3.93%     
==========================================
  Files         957      957              
  Lines       47002    46992      -10     
  Branches     4589     4589              
==========================================
- Hits        31753    29901    -1852     
- Misses      15137    16907    +1770     
- Partials      112      184      +72     
Flag Coverage Δ
cypress ?
javascript 62.68% <ø> (ø)
python 64.20% <96.72%> (-0.16%) ⬇️

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

Impacted Files Coverage Δ
superset/utils/log.py 92.98% <94.28%> (-0.23%) ⬇️
superset/views/base_api.py 98.14% <100.00%> (-0.06%) ⬇️
superset/views/core.py 75.22% <100.00%> (ø)
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupColors.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/chart/ChartContainer.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/reducers/index.js 0.00% <0.00%> (-100.00%) ⬇️
...et-frontend/src/dashboard/containers/Dashboard.jsx 0.00% <0.00%> (-100.00%) ⬇️
... and 179 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 77cae64...5ca1d1f. Read the comment docs.

@ktmud ktmud force-pushed the remove--path-no-int branch from 2979fe8 to ee58f18 Compare December 4, 2020 18:42
@pull-request-size pull-request-size bot added size/XL and removed size/M labels Dec 4, 2020
@ktmud ktmud changed the base branch from erik-ritter--remove-logs-columns to master December 4, 2020 18:42
@pull-request-size pull-request-size bot added size/M and removed size/XL labels Dec 4, 2020
@ktmud ktmud force-pushed the remove--path-no-int branch from ee58f18 to e94abbd Compare December 4, 2020 18:49
@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 4, 2020
@ktmud ktmud force-pushed the remove--path-no-int branch 2 times, most recently from 63db218 to dbee334 Compare December 4, 2020 20:22
@ktmud ktmud force-pushed the remove--path-no-int branch 3 times, most recently from edb3db5 to 788d5e4 Compare December 8, 2020 21:20
@ktmud ktmud changed the title feat: optimize backend log payload refactor: optimize backend log payload Dec 8, 2020
@ktmud ktmud requested review from mistercrunch, etr2460 and john-bodley and removed request for mistercrunch December 10, 2020 00:03
@ktmud ktmud force-pushed the remove--path-no-int branch 2 times, most recently from 1e71b7d to 72ce5af Compare December 11, 2020 20:09
@ktmud
Copy link
Member Author

ktmud commented Dec 14, 2020

@mistercrunch @etr2460 any thoughts on this one?

@ktmud ktmud force-pushed the remove--path-no-int branch from 72ce5af to b905dd1 Compare December 15, 2020 19:14
Copy link

@graceguo-supercat graceguo-supercat left a comment

Choose a reason for hiding this comment

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

LGTM

@ktmud ktmud force-pushed the remove--path-no-int branch from b905dd1 to 5ca1d1f Compare December 15, 2020 23:26
@ktmud ktmud merged commit 76f9f18 into apache:master Dec 16, 2020
@ktmud ktmud deleted the remove--path-no-int branch December 16, 2020 01:22
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.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.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants