feat(aci): Context logging #93617
feat(aci): Context logging #93617
Conversation
github-actions
bot
added
the
Scope: Backend
|
Note: I've applied this widely for demonstration purposes, but I'm happy to revert any of that for initial landing. |
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #93617 +/- ##
========================================
Coverage 88.03% 88.03%
========================================
Files 10328 10330 +2
Lines 596106 596252 +146
Branches 23146 23146
========================================
+ Hits 524760 524900 +140
- Misses 70890 70896 +6
Partials 456 456 |
There was a problem hiding this comment.
overall this lgtm as a generic logger solution.
i think we might want to introduce a little magic in a follow up PR to simplify the using the logger; either configuring the logger at the beginning of each processor method or have helpers use context vars.
i think we'll also need to think about the logger.debug vs logger.info etc, it's assuming correct use of each log level throughout. we could create a helper method to ensure that info / debug logs are locked down to orgs opt'd in (just for workflow engine).
| @@ -247,7 +248,9 @@ def process_workflows(event_data: WorkflowEventData) -> set[Workflow]: | |||
| """ | |||
| try: | |||
| detector = get_detector_by_event(event_data) | |||
| log_context.add_extras(detector_id=detector.id) | |||
There was a problem hiding this comment.
how are you thinking add_extras might work with WorkflowEventContext? would that be included initially?
| if features.has("projects:num-events-issue-debugging", project) or features.has( | ||
| "organizations:workflow-engine-process-workflows", project.organization | ||
| ): | ||
| log_context.set_verbose(True) |
There was a problem hiding this comment.
I like this format for a general purpose logger -- is there a way we can simplify adoption of this inside of workflow engine so we can easily filter down to specific orgs by just setting a flag?
one thought i had around this is that we could initialize the logger with this context data in the processing methods? 🤔
| _log_context_state.get().verbose = verbose | ||
|
|
||
|
|
||
| def add_extras(**extras: Any) -> None: |
There was a problem hiding this comment.
iirc we had discussed adding a trace_id or something along those lines to track individual evaluations. how are you thinking that would work with .add_extras?
There was a problem hiding this comment.
We currently have context_id automatically annotated in all of these when there's a log root. Are there cases not covered by that where we want an ID?
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Add helpers for automatically adjusting logging based on context. The aim here is to lean firmly toward keeping our scopes safe with root designations and context managers; better no annotations than wrong annotations.
Add helpers for automatically adjusting logging based on context.
The aim here is to lean firmly toward keeping our scopes safe with root designations and context managers; better no annotations than wrong annotations.