chore: address PR comments

Author: czi-fsisenda

sdks/python/src/learning_commons_evaluators/schemas/config.py

@@ -16,6 +16,9 @@
 
 DEFAULT_TELEMETRY_EVENTS_ENDPOINT = "https://api.learningcommons.org/evaluators-telemetry/v1/events"
 
+# Shared per process so multiple :class:`EvaluatorConfig` instances derive the same client id.
+_PROCESS_CLIENT_ID_SEED = uuid.uuid4()
+
 # --- LLM provider configs (for LLM calls in prompt steps) ---
 
 
@@ -112,8 +115,8 @@ class EvaluatorConfig:
 
     # Temporary until we finalize the telemetry API key/client id strategy.
     #: UUID v5 namespace for deriving ``X-Client-ID`` when ``telemetry_partner_id`` is an API key.
-    #: A new value is generated for each :class:`EvaluatorConfig` unless you pass one explicitly.
-    client_id_seed: uuid.UUID = field(default_factory=uuid.uuid4)
+    #: Defaults to a single per-process seed so all configs in one run share the same derived id.
+    client_id_seed: uuid.UUID = field(default=_PROCESS_CLIENT_ID_SEED)
 
 
 def create_config(

sdks/python/src/learning_commons_evaluators/telemetry/__init__.py

@@ -5,14 +5,16 @@
 import asyncio
 import threading
 import uuid
+from concurrent.futures import ThreadPoolExecutor
+from datetime import datetime, timezone
 
 import httpx
 
 from learning_commons_evaluators.schemas.config import EvaluatorConfig
 from learning_commons_evaluators.schemas.evaluator import EvaluationInput
 from learning_commons_evaluators.schemas.metadata import EvaluationMetadata
 from learning_commons_evaluators.telemetry.adapter import evaluation_to_typescript_telemetry_event
-from learning_commons_evaluators.telemetry.utils import client_id_from_seed
+from learning_commons_evaluators.telemetry.utils import client_id_from_seed, iso_utc_z
 
 __all__ = [
     "evaluation_to_typescript_telemetry_event",
@@ -21,6 +23,20 @@
     "should_send_telemetry",
 ]
 
+_TELEMETRY_EXECUTOR: ThreadPoolExecutor | None = None
+_TELEMETRY_EXECUTOR_LOCK = threading.Lock()
+
+
+def _get_telemetry_executor() -> ThreadPoolExecutor:
+    global _TELEMETRY_EXECUTOR
+    with _TELEMETRY_EXECUTOR_LOCK:
+        if _TELEMETRY_EXECUTOR is None:
+            _TELEMETRY_EXECUTOR = ThreadPoolExecutor(
+                max_workers=2,
+                thread_name_prefix="lc-telemetry",
+            )
+    return _TELEMETRY_EXECUTOR
+
 
 def should_send_telemetry(config: EvaluatorConfig) -> bool:
     """Return True when telemetry is configured with a non-empty partner / client id."""
@@ -47,54 +63,59 @@ async def send_telemetry(
     if not should_send_telemetry(config):
         return
 
-    partner_id = config.telemetry.telemetry_partner_id
-    assert (
-        partner_id is not None
-    )  # for mypy: ``should_send_telemetry`` guarantees non-empty after strip.
-    telemetry_partner_id = partner_id.strip()
-
-    event = evaluation_to_typescript_telemetry_event(evaluation_metadata, inp, config)
-    payload = event.model_dump(mode="json", exclude_none=True)
-
-    api_key = telemetry_partner_id if not _is_uuid(telemetry_partner_id) else None
-    client_id = (
-        telemetry_partner_id
-        if _is_uuid(telemetry_partner_id)
-        else client_id_from_seed(telemetry_partner_id, config.client_id_seed)
-    )
-
-    headers: dict[str, str] = {
-        "Content-Type": "application/json",
-        "X-Client-ID": client_id,
-    }
-    if api_key is not None:
-        headers["X-API-Key"] = api_key
-
     try:
+        partner_id = config.telemetry.telemetry_partner_id
+        assert (
+            partner_id is not None
+        )  # for mypy: ``should_send_telemetry`` guarantees non-empty after strip.
+        telemetry_partner_id = partner_id.strip()
+
+        event = evaluation_to_typescript_telemetry_event(evaluation_metadata, inp, config)
+        # TS SDK sets timestamp at send time (`new Date().toISOString()`), not evaluation start.
+        event = event.model_copy(update={"timestamp": iso_utc_z(datetime.now(timezone.utc))})
+        payload = event.model_dump(mode="json", exclude_none=True)
+
+        api_key = telemetry_partner_id if not _is_uuid(telemetry_partner_id) else None
+        client_id = (
+            telemetry_partner_id
+            if _is_uuid(telemetry_partner_id)
+            else client_id_from_seed(telemetry_partner_id, config.client_id_seed)
+        )
+
+        headers: dict[str, str] = {
+            "Content-Type": "application/json",
+            "X-Client-ID": client_id,
+        }
+        if api_key is not None:
+            headers["X-API-Key"] = api_key
+
         timeout = httpx.Timeout(5.0)
         async with httpx.AsyncClient(timeout=timeout) as client:
             response = await client.post(config.telemetry.endpoint, json=payload, headers=headers)
             if response.is_error:
+                # Log status only; response bodies may echo input text or other sensitive data.
                 config.logger.warning(
-                    "telemetry send failed: %s %s",
+                    "telemetry send failed: HTTP %s",
                     response.status_code,
-                    response.text[:500],
                 )
-    except httpx.RequestError as e:
-        config.logger.warning("telemetry send failed: %s", e)
+    except Exception as e:
+        # Log exception type only; ``str(e)`` may include payload fields (e.g. input_text).
+        config.logger.warning(
+            "telemetry send failed: %s",
+            type(e).__qualname__,
+        )
 
 
 def schedule_send_telemetry(
     evaluation_metadata: EvaluationMetadata,
     inp: EvaluationInput | None,
     config: EvaluatorConfig,
 ) -> None:
-    """Fire-and-forget: run :func:`send_telemetry` on a daemon thread when telemetry is enabled."""
+    """Fire-and-forget: run :func:`send_telemetry` on a shared worker when telemetry is enabled."""
     if not should_send_telemetry(config):
         return
 
     def _run() -> None:
         asyncio.run(send_telemetry(evaluation_metadata, inp, config))
 
-    thread = threading.Thread(target=_run, daemon=True)
-    thread.start()
+    _get_telemetry_executor().submit(_run)

sdks/python/src/learning_commons_evaluators/telemetry/adapter.py

@@ -19,7 +19,6 @@
 
 from __future__ import annotations
 
-from datetime import datetime, timezone
 from typing import Any
 
 from learning_commons_evaluators.schemas.config import EvaluatorConfig, LLMProvider
@@ -41,11 +40,7 @@
     TelemetryStageDetail,
     TelemetryTokenUsage,
 )
-
-
-def _iso_utc_z(dt: datetime) -> str:
-    s = dt.astimezone(timezone.utc).isoformat()
-    return s.replace("+00:00", "Z")
+from learning_commons_evaluators.telemetry.utils import iso_utc_z
 
 
 def _map_status(status: Status) -> EvaluationTelemetryStatus:
@@ -179,7 +174,7 @@ def evaluation_to_typescript_telemetry_event(
     )
 
     return TelemetryEvent(
-        timestamp=_iso_utc_z(evaluation_metadata.timestamp),
+        timestamp=iso_utc_z(evaluation_metadata.timestamp),
         sdk_version=evaluation_metadata.evaluator_metadata.sdk_version,
         evaluator_type=evaluation_metadata.evaluator_metadata.id,
         grade=grade,

sdks/python/src/learning_commons_evaluators/telemetry/utils.py

@@ -3,6 +3,13 @@
 from __future__ import annotations
 
 import uuid
+from datetime import datetime, timezone
+
+
+def iso_utc_z(dt: datetime) -> str:
+    """Format *dt* as an ISO 8601 UTC string with a ``Z`` suffix (TS wire format)."""
+    s = dt.astimezone(timezone.utc).isoformat()
+    return s.replace("+00:00", "Z")
 
 
 def client_id_from_seed(learning_commons_api_key: str, client_id_seed: uuid.UUID) -> str:

sdks/python/tests/schemas/test_config.py

@@ -72,6 +72,11 @@ def test_create_config_sets_telemetry_partner_id(self):
         assert config.telemetry.endpoint == DEFAULT_TELEMETRY_EVENTS_ENDPOINT
         assert config.telemetry.send_full_input_with_telemetry is False
 
+    def test_default_client_id_seed_shared_across_configs(self):
+        a = create_config(telemetry_partner_id="tid-a")
+        b = create_config(telemetry_partner_id="tid-b")
+        assert a.client_id_seed == b.client_id_seed
+
     def test_create_config_telemetry_with_full_input_sets_flag(self):
         config = create_config_telemetry_with_full_input(telemetry_partner_id="tid")
         assert config.telemetry.telemetry_partner_id == "tid"

sdks/python/tests/telemetry/test_telemetry.py

@@ -3,6 +3,7 @@
 from __future__ import annotations
 
 import asyncio
+from datetime import datetime, timezone
 from unittest.mock import AsyncMock, MagicMock, patch
 
 from learning_commons_evaluators import (
@@ -49,22 +50,21 @@ def test_true_with_partner_id(self):
 
 
 class TestScheduleSendTelemetry:
-    def test_does_not_spawn_thread_when_partner_missing(self, config, evaluation_metadata):
-        with patch("learning_commons_evaluators.telemetry.threading.Thread") as mock_thread:
+    def test_does_not_submit_when_partner_missing(self, config, evaluation_metadata):
+        with patch("learning_commons_evaluators.telemetry._get_telemetry_executor") as mock_exec:
             schedule_send_telemetry(evaluation_metadata, None, config)
-        mock_thread.assert_not_called()
+        mock_exec.assert_not_called()
 
-    @patch("learning_commons_evaluators.telemetry.threading.Thread")
-    def test_starts_daemon_thread_when_no_running_loop(self, mock_thread, evaluation_metadata):
+    @patch("learning_commons_evaluators.telemetry._get_telemetry_executor")
+    def test_submits_to_shared_executor(self, mock_get_executor, evaluation_metadata):
         cfg = create_config(telemetry_partner_id="tid")
-        mock_instance = MagicMock()
-        mock_thread.return_value = mock_instance
+        mock_executor = MagicMock()
+        mock_get_executor.return_value = mock_executor
 
         schedule_send_telemetry(evaluation_metadata, None, cfg)
 
-        mock_thread.assert_called_once()
-        assert mock_thread.call_args.kwargs.get("daemon") is True
-        mock_instance.start.assert_called_once()
+        mock_get_executor.assert_called_once()
+        mock_executor.submit.assert_called_once()
 
 
 class TestSendTelemetryHttp:
@@ -168,3 +168,40 @@ def test_send_telemetry_no_op_without_partner(
     ):
         asyncio.run(send_telemetry(evaluation_metadata, None, config))
         mock_client_class.assert_not_called()
+
+    @patch("learning_commons_evaluators.telemetry.httpx.AsyncClient")
+    def test_timestamp_is_set_at_send_time_not_evaluation_start(
+        self, mock_client_class, evaluation_metadata
+    ):
+        _mock_async_httpx_client(mock_client_class)
+        cfg = create_config(telemetry_partner_id="tid")
+        evaluation_metadata.timestamp = datetime(2020, 1, 1, tzinfo=timezone.utc)
+        evaluation_metadata.status = Status.succeeded
+
+        before = datetime.now(timezone.utc)
+        asyncio.run(send_telemetry(evaluation_metadata, None, cfg))
+        after = datetime.now(timezone.utc)
+
+        payload = mock_client_class.return_value.__aenter__.return_value.post.call_args[1]["json"]
+        sent = datetime.fromisoformat(payload["timestamp"].replace("Z", "+00:00"))
+        assert before <= sent <= after
+
+    @patch(
+        "learning_commons_evaluators.telemetry.evaluation_to_typescript_telemetry_event",
+        side_effect=RuntimeError("adapter blew up"),
+    )
+    def test_send_telemetry_swallows_non_http_errors(self, _mock_adapter, evaluation_metadata):
+        mock_logger = MagicMock()
+        cfg = create_config(telemetry_partner_id="tid", logger=mock_logger)
+        asyncio.run(send_telemetry(evaluation_metadata, None, cfg))
+        mock_logger.warning.assert_called_once()
+
+
+class TestClientIdSeed:
+    def test_same_partner_id_across_configs_yields_same_client_id(self):
+        cfg_a = create_config(telemetry_partner_id="my-key")
+        cfg_b = create_config(telemetry_partner_id="my-key")
+        assert cfg_a.client_id_seed == cfg_b.client_id_seed
+        assert client_id_from_seed("my-key", cfg_a.client_id_seed) == client_id_from_seed(
+            "my-key", cfg_b.client_id_seed
+        )