Skip to content

Handle numeric-string timestamps in thermostat MetricSample payloads#3791

Open
MontrealAI wants to merge 1 commit into
mainfrom
codex/fix-code-according-to-best-practices-231vyg
Open

Handle numeric-string timestamps in thermostat MetricSample payloads#3791
MontrealAI wants to merge 1 commit into
mainfrom
codex/fix-code-according-to-best-practices-231vyg

Conversation

@MontrealAI
Copy link
Copy Markdown
Owner

Motivation

  • MetricSample.from_payload previously treated string timestamps as ISO only, causing numeric-string epoch values to be mis-parsed and fallback to the current time.
  • Robust ingestion of metrics (Prometheus/NDJSON) must accept epoch seconds passed as strings to avoid silent timestamp corruption.
  • Fixing this reduces surprises in replay and live thermostat workflows that consume external telemetry.

Description

  • Detect numeric-string timestamps with a regular expression and parse them as epoch seconds in MetricSample.from_payload in services/thermostat/metrics.py.
  • Preserve existing behavior for ISO-8601 parsing and Z suffix handling as a fallback when the string is not numeric.
  • Add a unit test test_metric_sample_parses_numeric_string_timestamp in services/thermostat/tests/test_metrics.py to cover the new behavior.
  • Minor import ordering cleanup in the modified file.

Testing

  • Ran the targeted tests with pytest -q services/thermostat/tests/test_metrics.py, which passed (3 passed).
  • The repository-wide Python tests were previously observed green (pytest -q produced 245 passed, 1 skipped before this change).

Codex Task

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1f880531c9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +48 to +49
if re.match(r"^[+-]?\d+(\.\d+)?$", normalized):
timestamp = datetime.fromtimestamp(float(normalized), tz=timezone.utc)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Guard numeric-string epoch parsing against out-of-range values

The new numeric-string path calls datetime.fromtimestamp without a try/except, so a large numeric string (common if a producer sends epoch milliseconds, e.g. "1714560000000") raises ValueError: year ... out of range and bubbles out of from_payload. Before this change, string timestamps that couldn’t be parsed fell back to now, so ingestion wouldn’t crash. This is a regression for any upstream that emits numeric strings outside the supported seconds range; consider catching ValueError/OverflowError here or normalizing ms/µs before calling fromtimestamp.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant