a case study on why you should not let your PR get 280 commits behind

api/src/opentrons/__init__.py

@@ -143,7 +143,7 @@ async def initialize() -> ThreadManagedHardware:
  """
  robot_conf = robot_configs.load()
  logging_config.log_init(robot_conf.log_level)
-
+ log.info(f"Logging Level: {robot_conf.log_level}")
  log.info(f"API server version: {version}")
  log.info(f"Robot Name: {name()}")
 

api/src/opentrons/util/performance_helpers.py

@@ -1,5 +1,6 @@
 """Performance helpers for tracking robot context."""
 
+import asyncio
 import functools
 from pathlib import Path
 from opentrons_shared_data.performance.dev_types import (
@@ -10,22 +11,22 @@
 from opentrons_shared_data.robot.dev_types import RobotTypeEnum
 from typing import Callable, Type
 from opentrons.config import (
- feature_flags as ff,
  get_performance_metrics_data_dir,
  robot_configs,
+ feature_flags as ff,
 )
 
 
 _should_track = ff.enable_performance_metrics(
  RobotTypeEnum.robot_literal_to_enum(robot_configs.load().model)
 )
-STORE_EACH = _should_track
-
 
 class StubbedTracker(SupportsTracking):
  """A stubbed tracker that does nothing."""
 
- def __init__(self, storage_location: Path, should_track: bool) -> None:
+ def __init__(
+ self, storage_location: Path, should_track: bool
+ ) -> None:
  """Initialize the stubbed tracker."""
  pass
 
@@ -64,7 +65,6 @@ def _get_robot_context_tracker() -> SupportsTracking:
  """Singleton for the robot context tracker."""
  global _robot_context_tracker
  if _robot_context_tracker is None:
- # TODO: replace with path lookup and should_store lookup
  _robot_context_tracker = package_to_use(
  get_performance_metrics_data_dir(), _should_track
  )
@@ -78,15 +78,12 @@ def track_analysis(func: F) -> F:
 
  # Typing a decorator that wraps a decorator with args, nope
  @functools.wraps(func)
- def wrapper(*args, **kwargs): # type: ignore # noqa: ANN002, ANN003, ANN201
- tracker = _get_robot_context_tracker()
- tracked_func = tracker.track(RobotContextState.ANALYZING_PROTOCOL)(func)
+ async def wrapper(*args, **kwargs): # type: ignore # noqa: ANN002, ANN003, ANN201
+ tracker: SupportsTracking = _get_robot_context_tracker()
 
- result = tracked_func(*args, **kwargs)
-
- if STORE_EACH:
+ try:
+ return await tracker.track(func, RobotContextState.ANALYZING_PROTOCOL, *args, **kwargs)
+ finally:
  tracker.store()
 
- return result
-
  return wrapper # type: ignore

api/tests/opentrons/cli/test_cli.py

@@ -39,12 +39,6 @@ def override_data_store(tmp_path: Path) -> Iterator[None]:
  context_tracker._store = old_store # type: ignore[attr-defined]
 
 
-@pytest.fixture
-def monkeypatch_set_store_each_to_true(monkeypatch: pytest.MonkeyPatch) -> None:
- """Override the STORE_EACH flag for the RobotContextTracker."""
- monkeypatch.setattr("opentrons.util.performance_helpers.STORE_EACH", True)
-
-
 def verify_metrics_store_file(file_path: Path, expected_length: int) -> None:
  """Verify that the metrics store file contains the expected number of lines."""
  with open(file_path, "r") as f:

performance-metrics/.gitignore

@@ -1 +1,2 @@
-.ruff_cache/
+.ruff_cache/
+.logs/

performance-metrics/Makefile

@@ -1,4 +1,35 @@
 include ../scripts/python.mk
+include ../scripts/push.mk
+
+ot_project := $(OPENTRONS_PROJECT)
+project_rs_default = $(if $(ot_project),$(ot_project),robot-stack)
+project_ir_default = $(if $(ot_project),$(ot_project),ot3)
+
+SHX := npx shx
+
+# Host key location for robot
+ssh_key ?= $(default_ssh_key)
+# Other SSH args for robot
+ssh_opts ?= $(default_ssh_opts)
+# Helper to safely bundle ssh options
+ssh_helper = $(if $(ssh_key),-i $(ssh_key)) $(ssh_opts)
+
+# Defined separately than the clean target so the wheel file doesn’t have to
+# depend on a PHONY target
+
+# Find the version of the wheel from git using a helper script. We
+# use python here so we can use the same version normalization that will be
+# used to create the wheel.
+wheel_file = dist/$(call python_get_wheelname,performance-metrics,$(project_rs_default),performance_metrics,$(BUILD_NUMBER))
+
+# Find the version of the sdist file from git using a helper script.
+sdist_file = dist/$(call python_get_sdistname,performance_metrics,$(project_rs_default),performance_metrics)
+
+
+clean_cmd = $(SHX) rm -rf 'build' '**/*.egg-info' '**/__pycache__' **/*.pyc '.mypy_cache' '.pytest_cache'
+clean_wheel_cmd = $(clean_cmd) dist/*.whl
+clean_sdist_cmd = $(clean_cmd) dist/*.tar.gz
+clean_all_cmd = $(clean_cmd) dist
 
 .PHONY: lint
 lint:
@@ -20,12 +51,72 @@ teardown:
 
 .PHONY: clean
 clean:
- rm -rf build dist *.egg-info .mypy_cache .pytest_cache src/performance_metrics.egg-info
+ $(clean_all_cmd)
 
 .PHONY: wheel
 wheel:
+ $(clean_wheel_cmd)
  $(python) setup.py $(wheel_opts) bdist_wheel
- rm -rf build
+ $(SHX) rm -rf build
+ $(SHX) ls dist
+
+.PHONY: sdist
+sdist: export OPENTRONS_PROJECT=$(project_rs_default)
+sdist:
+ $(clean_sdist_cmd)
+ $(python) setup.py sdist
+ $(SHX) rm -rf build
+ $(SHX) ls dist
+
+.PHONY: push-no-restart
+push-no-restart: wheel
+ $(call push-python-package,$(host),$(ssh_key),$(ssh_opts),$(wheel_file))
+
+.PHONY: push
+push: push-no-restart
+ $(call restart-service,$(host),$(ssh_key),$(ssh_opts),"opentrons-robot-server")
+
+.PHONY: push-no-restart-ot3
+push-no-restart-ot3: sdist
+ $(call push-python-sdist,$(host),$(ssh_key),$(ssh_opts),$(sdist_file),"/opt/opentrons-robot-server","performance_metrics",,,$(version_file))
+
+.PHONY: push-ot3
+push-ot3: push-no-restart-ot3
+ $(call restart-server,$(host),$(ssh_key),$(ssh_opts),"opentrons-robot-server")
+
+.PHONY: set-debug-log-level
+set-debug-log-level:
+ @echo "Setting log level to debug on robot"
+ curl \
+ -X POST\
+ -H "opentrons-version: *" \
+ -H "Content-Type: application/json" \
+ -d '{"log_level": "debug"}' \
+ http:https://$(host):31950/settings/log_level/local | jq
+
+.PHONY: add-performance-metrics-ff
+add-performance-metrics-ff:
+ @echo "Adding performance metrics feature flag to robot"
+ ssh -i $(ssh_key) $(ssh_opts) root@$(host) "touch /data/robot.env && \
+ grep -q 'OT_API_FF_enablePerformanceMetrics' /data/robot.env && \
+ sed -i 's/OT_API_FF_enablePerformanceMetrics=false/OT_API_FF_enablePerformanceMetrics=true/' /data/robot.env || \
+ echo 'OT_API_FF_enablePerformanceMetrics=true' \
+ >> /data/robot.env"
+
+
+.PHONY: get-logs
+get-logs:
+ @echo "Creating logs directory"
+ mkdir -p .logs
+
+ @echo "Getting logs from robot"
+ ssh -i $(ssh_key) $(ssh_opts) root@$(host) "journalctl \
+ --no-pager \
+ --boot \
+ --catalog \
+ --unit opentrons-robot-server.service" \
+ > .logs/robot-server.log
+
 
 .PHONY: test
 test:

performance-metrics/pyproject.toml

@@ -0,0 +1,3 @@
+[build-system]
+requires = ["setuptools", "wheel"]
+build-backend = "setuptools.build_meta:__legacy__"

performance-metrics/setup.cfg

@@ -0,0 +1,11 @@
+[bdist_wheel]
+universal = 1
+
+[metadata]
+license_files = ../LICENSE
+
+[pep8]
+ignore = E221,E222
+
+[aliases]
+test=pytest

performance-metrics/src/performance_metrics/robot_context_tracker.py

@@ -1,5 +1,6 @@
 """Module for tracking robot context and execution duration for different operations."""
 
+import inspect
 from pathlib import Path
 import platform
 
@@ -48,7 +49,7 @@ class RobotContextTracker(SupportsTracking):
 
  METADATA_NAME: Final[Literal["robot_context_data"]] = "robot_context_data"
 
- def __init__(self, storage_location: Path, should_track: bool = False) -> None:
+ def __init__(self, storage_location: Path, should_track: bool) -> None:
  """Initializes the RobotContextTracker with an empty storage list."""
  self._store = MetricsStore[RawContextData](
  MetricsMetadata(
@@ -62,7 +63,7 @@ def __init__(self, storage_location: Path, should_track: bool = False) -> None:
  if self._should_track:
  self._store.setup()
 
- def track(self, state: RobotContextState) -> Callable: # type: ignore
+ async def track(self, func_to_track: Callable, state: RobotContextState, *args, **kwargs) -> Callable: # type: ignore
  """Decorator factory for tracking the execution duration and state of robot operations.
 
  Args:
@@ -72,31 +73,35 @@ def track(self, state: RobotContextState) -> Callable: # type: ignore
  Callable: A decorator that wraps a function to track its execution duration and state.
  """
 
- def inner_decorator(func: Callable[P, R]) -> Callable[P, R]:
- if not self._should_track:
- return func
-
- @wraps(func)
- def wrapper(*args: P.args, **kwargs: P.kwargs) -> R:
- function_start_time = timing_function()
- duration_start_time = perf_counter_ns()
- try:
- result = func(*args, **kwargs)
- finally:
- duration_end_time = perf_counter_ns()
- self._store.add(
- RawContextData(
- func_start=function_start_time,
- duration=duration_end_time - duration_start_time,
- state=state,
- )
- )
-
- return result
-
- return wrapper
-
- return inner_decorator
+
+
+ if not self._should_track:
+ return func_to_track
+
+ function_start_time = timing_function()
+ duration_start_time = perf_counter_ns()
+
+ if inspect.iscoroutinefunction(func_to_track):
+ try:
+ result = await func_to_track(*args, **kwargs)
+ finally:
+ duration_end_time = perf_counter_ns()
+ else:
+ try:
+ result = func_to_track(*args, **kwargs)
+ finally:
+ duration_end_time = perf_counter_ns()
+
+ self._store.add(
+ RawContextData(
+ func_start=function_start_time,
+ duration=duration_end_time - duration_start_time,
+ state=state,
+ )
+ )
+ return result
+
+
 
  def store(self) -> None:
  """Returns the stored context data and clears the storage list."""

performance-metrics/tests/performance_metrics/conftest.py

@@ -0,0 +1,16 @@
+"""Shared fixtures and functions for performance metrics tests."""
+
+from pathlib import Path
+
+
+def verify_metrics_store_file(file_path: Path, expected_length: int) -> None:
+ """Verify that the metrics store file contains the expected number of lines."""
+ with open(file_path, "r") as f:
+ stored_data = f.readlines()
+ stored_data = [line.strip() for line in stored_data if line.strip()]
+ assert len(stored_data) == expected_length
+ for line in stored_data:
+ state_id, start_time, duration = line.strip().split(",")
+ assert state_id.isdigit()
+ assert start_time.isdigit()
+ assert duration.isdigit()

robot-server/robot_server/protocols/protocol_analyzer.py

@@ -5,6 +5,7 @@
 from opentrons import protocol_runner
 from opentrons.protocol_engine.errors import ErrorOccurrence
 from opentrons.protocol_engine.types import RunTimeParamValuesType
+from opentrons.util.performance_helpers import track_analysis
 import opentrons.util.helpers as datetime_helper
 
 import robot_server.errors.error_mappers as em
@@ -25,6 +26,7 @@ def __init__(
  """Initialize the analyzer and its dependencies."""
  self._analysis_store = analysis_store
 
+ @track_analysis
  async def analyze(
  self,
  protocol_resource: ProtocolResource,