feat: global logs context #26418
feat: global logs context #26418
Conversation
eschutho
force-pushed
the
elizabeth/global-logs-context
branch
2 times, most recently
from
c88d248 to
5d40fd9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #26418 +/- ##
==========================================
- Coverage 69.14% 69.08% -0.07%
==========================================
Files 1946 1946
Lines 75990 76041 +51
Branches 8479 8479
==========================================
- Hits 52544 52531 -13
- Misses 21267 21331 +64
Partials 2179 2179
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
eschutho
force-pushed
the
elizabeth/global-logs-context
branch
from
5d40fd9 to
3f0742d
Compare
eschutho
force-pushed
the
elizabeth/global-logs-context
branch
2 times, most recently
from
dcc0967 to
6988b1d
Compare
superset/utils/decorators.py
Outdated
| "slice_id", | ||
| "dashboard_id", | ||
| "execution_id", | ||
| "report_schedule_id", |
There was a problem hiding this comment.
Curious, why do we have a list of allowed keys? Is to prevent logging self?
There was a problem hiding this comment.
it's to prevent people from logging any unnecessary information or to avoid antipatterns. There's prob more context in the SIP.
There was a problem hiding this comment.
I left a comment in the code for anyone else who may have the same question.
superset/utils/decorators.py
Outdated
| if key in locals()["kwargs"]: | ||
| context_data = {key: locals()["kwargs"][key]} |
There was a problem hiding this comment.
You should be able to use kwargs directly here, no? Instead of locals()["kwargs"].
There was a problem hiding this comment.
Thanks! yep, that worked.
| from superset.reports.notifications.slack import SlackNotification, WebClient | ||
|
|
||
| execution_id = uuid.uuid4() | ||
| g.logs_context = {"execution_id": execution_id} |
There was a problem hiding this comment.
Can you patch superset.utils.decorators.g instead of relying on the actual g? Something like this:
patch("superset.utils.decorator.g.logs_context", new_callable=dict)Or maybe patch in the origin:
patch("flask.g.logs_context", new_callable=dict)Ditto in test_context_decorator below.
There was a problem hiding this comment.
Good call. I updated this too.
eschutho
force-pushed
the
elizabeth/global-logs-context
branch
3 times, most recently
from
1cc6846 to
3df2191
Compare
eschutho
force-pushed
the
elizabeth/global-logs-context
branch
from
3df2191 to
519ea97
Compare
eschutho
force-pushed
the
elizabeth/global-logs-context
branch
from
bc2b552 to
329f684
Compare
(cherry picked from commit aaa4a7b)
mistercrunch
added
🏷️ bot
SUMMARY
This is the first PR for #26019 [SIP-109] Global logs context.
It introduces a decorator
logs_contextthat will add permitted values to a global variablelogs_contextthat can later be retrieved for logging information when a larger context is needed than what is available to the method that is calling the logging function. I included an example for slack notifications, but there are more examples in my POC/draft pr #25920 that has more examples of how we can use this decorator.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION