betodealmeida commented on a change in pull request #13441:
URL: https://github.com/apache/superset/pull/13441#discussion_r587723926
##########
File path: superset/utils/log.py
##########
@@ -133,8 +154,29 @@ def log_context( # pylint: disable=too-many-locals
records=records,
dashboard_id=dashboard_id,
slice_id=slice_id,
- duration_ms=round((time.time() - start_time) * 1000),
+ duration_ms=int(duration.total_seconds() * 1000),
referrer=referrer,
+ **kwargs,
+ )
+
+ @contextmanager
+ def log_context( # pylint: disable=too-many-locals
+ self, action: str, object_ref: Optional[str] = None, log_to_statsd:
bool = True,
+ ) -> Iterator[Callable[..., None]]:
+ """
+ Log an event with additional information from the request context.
+ :param action: a name to identify the event
+ :param object_ref: reference to the Python object that triggered this
action
+ :param log_to_statsd: whether to update statsd counter for the action
+ """
+ payload_override = {}
+ start = datetime.now()
+ # yield a helper to add additional payload
+ yield lambda **kwargs: payload_override.update(kwargs)
+ duration = datetime.now() - start
+
+ self.log_with_context(
+ action, duration, object_ref, log_to_statsd, payload_override
Review comment:
```suggestion
action, duration, object_ref, log_to_statsd, **payload_override
```
See my comment above.
##########
File path: superset/utils/log.py
##########
@@ -72,32 +100,25 @@ def log( # pylint: disable=too-many-arguments
) -> None:
pass
- @contextmanager
- def log_context( # pylint: disable=too-many-locals
- self, action: str, object_ref: Optional[str] = None, log_to_statsd:
bool = True,
- ) -> Iterator[Callable[..., None]]:
- """
- Log an event with additional information from the request context.
-
- :param action: a name to identify the event
- :param object_ref: reference to the Python object that triggered this
action
- :param log_to_statsd: whether to update statsd counter for the action
- """
+ def log_with_context(
+ self,
+ action: str,
+ duration: timedelta,
+ object_ref: Optional[str] = None,
+ log_to_statsd: bool = True,
+ payload_override: Optional[Dict[str, Any]] = None,
+ **kwargs: Any,
Review comment:
I think it would be nice to have `payload_override` be passed like we're
doing in the context manager:
```suggestion
**payload_override: Optional[Dict[str, Any]],
```
This way if we don't want to use the context manager we can do:
```python
event_logger.log_with_context(action="foo", engine="bar")
```
##########
File path: superset/utils/log.py
##########
@@ -58,6 +59,33 @@ def collect_request_payload() -> Dict[str, Any]:
class AbstractEventLogger(ABC):
+ def __call__(
+ self,
+ action: str,
+ object_ref: Optional[str] = None,
+ log_to_statsd: bool = True,
+ duration: Optional[timedelta] = None,
+ **payload_override: Dict[str, Any],
+ ) -> object:
+ self.action = action
+ self.object_ref = object_ref
+ self.log_to_statsd = log_to_statsd
+ self.payload_override = payload_override
+ return self
+
+ def __enter__(self) -> None:
+ self.start = datetime.now()
+
+ def __exit__(self, exc_type: Any, exc_val: Any, exc_tb: Any) -> None:
+ # Log data w/ arguments being passed in
+ self.log_with_context(
+ action=self.action,
+ object_ref=self.object_ref,
+ log_to_statsd=self.log_to_statsd,
+ duration=datetime.now() - self.start,
+ payload_override=self.payload_override,
Review comment:
```suggestion
**self.payload_override,
```
See my comment below.
##########
File path: tests/event_logger_tests.py
##########
@@ -101,3 +110,40 @@ def test_func(arg1, add_extra_log_payload, karg1=1):
],
)
self.assertGreaterEqual(payload["duration_ms"], 100)
+
+ @patch("superset.utils.log.g", spec={})
+ @freeze_time("Jan 14th, 2020", auto_tick_seconds=15)
+ def test_context_manager_log(self, mock_g):
+ class DummyEventLogger(AbstractEventLogger):
+ def __init__(self):
+ self.records = []
+
+ def log(
+ self,
+ user_id: Optional[int],
+ action: str,
+ dashboard_id: Optional[int],
+ duration_ms: Optional[int],
+ slice_id: Optional[int],
+ referrer: Optional[str],
+ *args: Any,
+ **kwargs: Any,
+ ):
+ self.records.append(
+ {**kwargs, "user_id": user_id, "duration": duration_ms}
+ )
+
+ logger = DummyEventLogger()
+
+ with app.test_request_context():
+ mock_g.user = security_manager.find_user("gamma")
+ with logger(action="foo", engine="bar"):
Review comment:
Can you add a test for `logger.log_with_context` as well?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
