Skip to content

feat: add lastRefreshTime to Load Request Success telemetry events#11283

Merged
RusovDmitriy merged 1 commit into
masterfrom
feat-propagate-lastRefreshTime
Jul 20, 2026
Merged

feat: add lastRefreshTime to Load Request Success telemetry events#11283
RusovDmitriy merged 1 commit into
masterfrom
feat-propagate-lastRefreshTime

Conversation

@RusovDmitriy

Copy link
Copy Markdown
Member

No description provided.

@RusovDmitriy
RusovDmitriy requested review from a team as code owners July 17, 2026 13:52
@github-actions github-actions Bot added the rust Pull requests that update Rust code label Jul 17, 2026
@RusovDmitriy
RusovDmitriy requested a review from paveltiunov July 17, 2026 13:52
@github-actions github-actions Bot added the javascript Pull requests that update Javascript code label Jul 17, 2026
@RusovDmitriy
RusovDmitriy requested a review from ovr July 17, 2026 13:52
@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RusovDmitriy's task in 1m 0s —— View job


Code Review

Review complete — verdict: LGTM with 2 minor observations. Click to expand.
  • Read PR diff
  • Review gateway.ts changes
  • Review Rust changes (scan.rs, service.rs, shim.rs, node_export.rs)
  • Check tests

Summary

Clean, well-scoped change. The Rust side threads lastRefreshTime from the load-result schema metadata through SpanId (min/oldest-wins semantics) and surfaces it on the Load Request Success telemetry emitted by both the postgres shim and native SQL handler. The JS gateway mirrors the same min-across-queries reduction. A unit test covers the SpanId min semantics including the "newer must not override older" case. Nice.

Observations (non-blocking)

1. packages/cubejs-api-gateway/src/gateway.ts:2095-2098 — NaN can slip past .filter(Boolean)

const lastRefreshTimestamps = results
  .map((r: any) => r.getRootResultObject()[0].lastRefreshTime)
  .filter(Boolean)
  .map((t: string) => new Date(t).getTime());

filter(Boolean) drops falsy strings, but new Date(t).getTime() runs after the filter — an unparseable-but-truthy string yields NaN, which poisons Math.min(...) and then throws RangeError: Invalid time value at new Date(NaN).toISOString(), killing the whole telemetry log line. In practice the value comes from Rust's to_rfc3339_opts, so this is unlikely to fire, but a defensive .filter(Number.isFinite) after the parse would make the log call bulletproof at zero cost.

2. rust/cubesql/cubesql/src/compile/engine/df/scan.rs:825 — only the first result contributes

load_data only takes result.into_iter().next() when recording the metadata, so if a load ever returns multiple results with distinct lastRefreshTime values, siblings are silently ignored. Matches the existing "first result wins" pattern in this function, so behavior is consistent — flagging only because the accompanying SpanId::set_last_refresh_time docstring implies aggregation across all contributors.

Strengths

  • SpanId::set_last_refresh_time correctly implements min-semantics (older-wins) with a good docstring explaining why.
  • Test at rust/cubesql/cubesql/src/transport/service.rs:1064 explicitly asserts that a later, newer value does not overwrite the recorded oldest — exactly the tricky case.
  • Same field name and ISO-8601 format (SecondsFormat::Millis, UTC) used on both JS and Rust telemetry paths, keeping downstream consumers uniform.
  • Change is additive; existing telemetry keys are untouched.
· branch: `feat-propagate-lastRefreshTime`

@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.48936% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.46%. Comparing base (0bdbe18) to head (3b5d3a2).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
packages/cubejs-api-gateway/src/gateway.ts 50.00% 1 Missing and 1 partial ⚠️
rust/cubesql/cubesql/src/compile/engine/df/scan.rs 80.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #11283   +/-   ##
=======================================
  Coverage   79.45%   79.46%           
=======================================
  Files         473      473           
  Lines       97597    97642   +45     
  Branches     3539     3540    +1     
=======================================
+ Hits        77545    77590   +45     
+ Misses      19546    19545    -1     
- Partials      506      507    +1     
Flag Coverage Δ
cube-backend 59.44% <50.00%> (-0.01%) ⬇️
cubesql 83.82% <95.34%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@RusovDmitriy
RusovDmitriy merged commit 802758c into master Jul 20, 2026
143 of 146 checks passed
@RusovDmitriy
RusovDmitriy deleted the feat-propagate-lastRefreshTime branch July 20, 2026 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

javascript Pull requests that update Javascript code rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants