Expose metaflow logger and monitor via singleton #1794

talsperre · 2024-04-08T01:08:48Z

Expose logger and monitor via a `system_current` singleton

Currently, users don't have the ability to use their implementation of logger and monitor sidecars in their own code. Additionally, if platform developers want to instrument their Metaflow extensions, they have to pass in the logger/monitor constructs to each one of their plugins, leading to code duplication.

This PR exposes two new singleton objects called _system_logger and _system_monitor that can be used to access the monitor and the logger anywhere.

The monitor/logger can be accessed both within and outside a Metaflow flow. This allows us to instrument Metaflow plugins like metaflow.S3, which is often used outside of a flow as well.

Usage

The monitor/logger sidecar can be used in the following manner:

with _system_monitor.count("<your_metric_name>"): 
      # your code
      pass

_system_logger.log(payload):

romain-intel

Some initial comments.

metaflow/task.py

romain-intel · 2024-04-11T16:49:36Z

metaflow/task.py

+ pass
+ system_current.logger.log(
+ {
+ "log_type": "ERROR",


could we have constants for these?

romain-intel · 2024-04-11T16:51:32Z

metaflow/metaflow_metrics_manager.py

+
+ @contextmanager
+ def measure(self, metric_name, qualifer_name=None):
+ timer, counter = self.monitor.get_measure_metrics(metric_name)


could we ideally make this look symetric for both?

we can also just return a single "payload". Internally, the send_metric would know to stop the timer for example.

romain-intel

A few initial comments but I think this goes in the direction we agreed to so we shouldn't be too far.

romain-intel · 2024-04-30T06:45:40Z

metaflow/__init__.py

@@ -100,6 +100,12 @@ class and related decorators.
 # current runtime singleton
 from .metaflow_current import current

+# system monitor runtime singleton


I would add a comment like (for internal metaflow use only). It's kind of explicit from the name but we can be extra explicit :)

romain-intel · 2024-04-30T06:46:59Z

metaflow/cli.py

@@ -1066,6 +1068,10 @@ def start(
 if decospecs:
 decorators._attach_decorators(ctx.obj.flow, decospecs)

+ # We create an instance of SystemMonitor and SystemLogger respectively


nit, I would move this up to where ctx.obj.monitor and event_logger are created.

romain-intel · 2024-04-30T06:50:14Z

metaflow/metaflow_system_logger.py

+
+ @property
+ def flow_name(self):
+ return self._flow_name


assumes that flow_name will not be called prior to flow which may not be correct. In general, I would have one init method that inits all 4 (flow, flow_name, environment and logger) and controlled by one single flag.

This comment is still not addressed I think.

romain-intel · 2024-04-30T06:51:09Z

metaflow/metaflow_system_logger.py

+ return self._logger
+
+ @logger.setter
+ def logger(self, logger):


we don't really want this to be settable independently do we? can we just keep set_logger and have things set directly on the _ names? Or is there a use you are thinking of of setting things independently?

romain-intel · 2024-04-30T06:51:29Z

metaflow/metaflow_system_logger.py

+from typing import Dict, Any
+
+
+class SystemLogger(object):


nit: please add type hints where appropriate.

romain-intel · 2024-04-30T06:52:20Z

metaflow/metaflow_system_monitor.py

+from contextlib import contextmanager
+
+
+class SystemMonitor(object):


same comments as for system_logger

romain-intel · 2024-04-30T06:55:08Z

metaflow/task.py

- "ts": round(time.time()),
- }
- logger.log(msg)
+ with _system_monitor.count("metaflow.task.start"):


I would keep things the same here -- ie: use self.event_logger and self.logger

romain-intel · 2024-04-30T06:55:17Z

metaflow/task.py

- "flow_name": self.flow.name,
- }
- logger.log(tsk_msg)
+ print("I am here in metaflow")


nit: remove.

romain-intel · 2024-04-30T06:56:00Z

metaflow/task.py

- "runtime": round(end),
- }
- logger.log(msg)
+ _system_monitor.gauge("metaflow.task.duration", duration)


A measure would be more appropriate here than a gauge.

Question is should we be measuring the task duration only until the task "ends" or consider the portion dealing with metadata register as part of the task duration as well?

If it's the former then it would be difficult to implement using the existing context manager construct - hence why we use the gauge metric.

That's a good point so we may need to add non-context specific ones for this. The issue is that a gauge isn't the same as a measure. Gauge are supposed to measure levels of things (like # of machines in a pool, etc) not disparate point measurements like this.

savingoyal · 2024-05-09T15:52:50Z

metaflow/metaflow_system_logger.py

+from typing import Dict, Any, Optional, Union
+
+
+class SystemLogger(object):


Consider moving this and monitor under metaflow/system so that we can have a rather clean import - from metaflow.system import monitor rather than from .metaflow_system_monitor import _system_monitor or from metaflow import _system_monitor

savingoyal · 2024-05-09T15:54:40Z

metaflow/metaflow_system_logger.py

+
+class SystemLogger(object):
+ def __init__(self):
+ self._flow = None


why both _flow and _flow_name?

savingoyal · 2024-05-09T15:55:37Z

metaflow/metaflow_system_logger.py

+from typing import Dict, Any, Optional, Union
+
+
+class SystemLogger(object):


It seems that the implementation of this file depends on its usage outside of the flow context. Can you add comments in the code with the use cases so that it is easier to review and maintain going forward?

savingoyal · 2024-05-09T15:56:48Z

metaflow/metaflow_system_logger.py

+ self._logger.start()
+ return self._logger
+
+ def set_logger(


Other elements besides logger are being set in this method. consider renaming?

savingoyal · 2024-05-09T15:59:50Z

metaflow/metaflow_system_logger.py

+ self._logger = LOGGING_SIDECARS[DEFAULT_EVENT_LOGGER](
+ flow=self.flow, env=self.environment
+ )
+ self._debug("Started logger outside of a flow")


Would it be cleaner if self._logger is started when logger is constructed?

savingoyal · 2024-05-09T16:08:15Z

metaflow/task.py

+ "project_flow_name": current.get("project_flow_name"),
+ "trace_id": trace_id or None,
+ }
+ self.event_logger.send(


why use send instead of log? iirc send was a workaround for a very specific use case (I need to dig through the slack conversations) and we would want to avoid introducing it too much in the code base.

metaflow/task.py

savingoyal · 2024-05-09T16:12:34Z

metaflow/task.py

+ "traceback": traceback.format_exc(),
+ }
+ )
+ pass


why is this needed?

Replace it with something like event_logger.log(msg="", type="").

savingoyal · 2024-05-09T16:14:22Z

metaflow/task.py

+ "event_value": 1,
+ }
+ )
+ pass


same here - why is a pass needed? is it to signify end of an indent block or something more? if the former, we can avoid introducing pass to maintain consistency of style in the code base

savingoyal · 2024-05-09T16:15:06Z

metaflow/task.py

+ with self.monitor.measure("metaflow.task.duration"):
+ try:
+ with self.monitor.count("metaflow.task.start"):
+ self.event_logger.log(


why is this log necessary?

talsperre requested review from romain-intel and savingoyal April 8, 2024 17:13

romain-intel reviewed Apr 11, 2024

View reviewed changes

talsperre added 5 commits April 29, 2024 10:21

Expose metaflow logger and monitor via singleton

b794e47

Update task metrics, add gauge functionality to metrics manager

fd2fdf3

add stub methods to null event logger

0da2c8a

Fix bug when monitor msg is none

7799ef0

Replace system current singleton with logger and monitor singletons

a5984a2

talsperre force-pushed the dev/sidecar-update branch from e35dd85 to a5984a2 Compare April 29, 2024 23:23

romain-intel reviewed Apr 30, 2024

View reviewed changes

Address comments

b23053e

savingoyal requested changes May 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose metaflow logger and monitor via singleton #1794

Expose metaflow logger and monitor via singleton #1794

talsperre commented Apr 8, 2024 •

edited

romain-intel left a comment

romain-intel Apr 11, 2024

romain-intel Apr 11, 2024

romain-intel Apr 11, 2024

romain-intel left a comment

romain-intel Apr 30, 2024

romain-intel Apr 30, 2024

romain-intel Apr 30, 2024

romain-intel May 6, 2024

romain-intel Apr 30, 2024

romain-intel Apr 30, 2024

romain-intel Apr 30, 2024

romain-intel Apr 30, 2024

romain-intel Apr 30, 2024

romain-intel Apr 30, 2024

talsperre Apr 30, 2024

romain-intel May 1, 2024

savingoyal May 9, 2024

savingoyal May 9, 2024

savingoyal May 9, 2024

savingoyal May 9, 2024

savingoyal May 9, 2024

savingoyal May 9, 2024

savingoyal May 9, 2024

talsperre May 16, 2024

savingoyal May 9, 2024

savingoyal May 9, 2024

		from contextlib import contextmanager


		class SystemMonitor(object):

		from typing import Dict, Any, Optional, Union


		class SystemLogger(object):

Expose metaflow logger and monitor via singleton #1794

Are you sure you want to change the base?

Expose metaflow logger and monitor via singleton #1794

Conversation

talsperre commented Apr 8, 2024 • edited

Expose logger and monitor via a system_current singleton

Usage

romain-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romain-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talsperre commented Apr 8, 2024 •

edited

Expose logger and monitor via a `system_current` singleton