Observability frontend update #318
Conversation
WalkthroughThe changes update both frontend and backend to replace timeseries handling with a drift-based approach. API endpoints now return drift data types, and obsolete timeseries methods are removed. New components and utilities for drift metrics are added while deprecated metric-type definitions and sample data utilities are eliminated. Routes and services are modified to use drift data and adjust UI headers accordingly. On the backend, related HTTP endpoints and handlers for timeseries have been removed. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant FR as Frontend Route
participant JS as Join Service
participant API as API (getJoinData)
U->>FR: Request join page
FR->>JS: Call getJoinData(driftMetric)
JS->>API: Fetch joinDrift data
API-->>JS: Return joinDrift response
JS-->>FR: Deliver join data
FR-->>U: Render page with drift data
sequenceDiagram
participant U as User
participant UI as DriftMetricToggle
participant QP as Query Params
U->>UI: Click drift metric button
UI->>QP: Update metric parameter
QP-->>UI: Return new drift metric
UI-->>U: Re-render with updated metric
Possibly related PRs
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsGitHub Actions and Pipeline Checks: Resource not accessible by integration - https://docs.github.com/rest/actions/workflow-runs#list-workflow-runs-for-a-repository. Please grant the required permissions to the CodeRabbit GitHub App under the organization or repository settings. ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
sean-zlai
force-pushed
the
sean/thrift-observability-apis
branch
from
af08dcb to
d634069
Compare
sean-zlai
force-pushed
the
sean/echarts-to-layerchart2
branch
from
91f1421 to
e76e315
Compare
sean-zlai
force-pushed
the
sean/thrift-observability-apis
branch
from
d634069 to
b440572
Compare
sean-zlai
force-pushed
the
sean/echarts-to-layerchart2
branch
from
6928aab to
1708448
Compare
sean-zlai
force-pushed
the
sean/thrift-observability-apis
branch
2 times, most recently
from
618bf14 to
12a2a46
Compare
| { label: 'p95', color: '#4B92FF', index: 19 }, | ||
| { label: 'p50', color: '#7DFFB3', index: 10 }, | ||
| { label: 'p5', color: '#FDDD61', index: 1 } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte (1)
21-28: Improve error handling in data loading.The catch block only logs errors. Consider showing an error state to users.
} catch (err) { console.error('Error loading distributions:', err); + isLoadingDistributions = false; + distributions = []; }frontend/src/lib/api/api.ts (1)
163-166: Improve empty response handling.The TODO comment indicates uncertainty about handling empty responses. Consider returning null instead of an empty object for better type safety.
if (text) { return JSON.parse(text) as Data; } else { - // TODO: Should we return `null` here and require users to handle - return {} as Data; + return null as Data; }
🧹 Nitpick comments (4)
frontend/src/lib/components/charts/PercentileLineChart.svelte (1)
27-29: Beware of hardcoded indexes.
Consider defining percentile indexes in a config to avoid coupling to “19”, “10”, and “1”.frontend/src/routes/joins/[slug]/observability/drift/+page.svelte (1)
335-363: Mock null values are temporary placeholders.frontend/src/routes/joins/[slug]/+layout.ts (1)
12-18: Consider moving jobTracker functionality now.Instead of leaving a TODO, consider moving the jobTracker functionality to the
job-trackingroute in this PR to maintain code organization.frontend/src/routes/joins/[slug]/observability/distributions/+page.ts (1)
10-26: Consider showing errored columns in UI.As noted in the TODO, showing errored columns in an error state would improve user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (22)
frontend/src/lib/api/api.ts(3 hunks)frontend/src/lib/components/ChartControls.svelte(2 hunks)frontend/src/lib/components/DriftMetricToggle.svelte(1 hunks)frontend/src/lib/components/MetricTypeToggle.svelte(0 hunks)frontend/src/lib/components/charts/FeaturesLineChart.svelte(3 hunks)frontend/src/lib/components/charts/PercentileLineChart.svelte(2 hunks)frontend/src/lib/types/MetricType/MetricType.ts(0 hunks)frontend/src/lib/types/Model/Model.ts(0 hunks)frontend/src/lib/util/drift-metric.ts(1 hunks)frontend/src/lib/util/sample-data.ts(0 hunks)frontend/src/lib/util/sort.ts(0 hunks)frontend/src/routes/+layout.ts(1 hunks)frontend/src/routes/joins/[slug]/+layout.svelte(1 hunks)frontend/src/routes/joins/[slug]/+layout.ts(1 hunks)frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte(4 hunks)frontend/src/routes/joins/[slug]/observability/distributions/+page.ts(1 hunks)frontend/src/routes/joins/[slug]/observability/drift/+page.svelte(9 hunks)frontend/src/routes/joins/[slug]/services/joins.service.ts(1 hunks)frontend/src/routes/thrift/+page.svelte(0 hunks)hub/src/main/java/ai/chronon/hub/HubVerticle.java(0 hunks)hub/src/main/scala/ai/chronon/hub/handlers/TimeSeriesHandler.scala(0 hunks)hub/src/test/scala/ai/chronon/hub/handlers/TimeSeriesHandlerTest.scala(0 hunks)
💤 Files with no reviewable changes (9)
- hub/src/main/java/ai/chronon/hub/HubVerticle.java
- frontend/src/lib/util/sort.ts
- hub/src/main/scala/ai/chronon/hub/handlers/TimeSeriesHandler.scala
- frontend/src/lib/components/MetricTypeToggle.svelte
- frontend/src/routes/thrift/+page.svelte
- hub/src/test/scala/ai/chronon/hub/handlers/TimeSeriesHandlerTest.scala
- frontend/src/lib/types/Model/Model.ts
- frontend/src/lib/types/MetricType/MetricType.ts
- frontend/src/lib/util/sample-data.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/src/routes/+layout.ts
🧰 Additional context used
📓 Learnings (3)
frontend/src/routes/joins/[slug]/+layout.svelte (1)
Learnt from: sean-zlai
PR: zipline-ai/chronon#244
File: frontend/src/routes/joins/[slug]/+layout.svelte:9-9
Timestamp: 2025-01-17T21:50:45.432Z
Learning: In Svelte 5, `$derived` is a new reactivity primitive that replaces the older reactive declarations. Don't suggest changing `$derived` expressions to `$store` syntax.
frontend/src/routes/joins/[slug]/+layout.ts (1)
Learnt from: ken-zlai
PR: zipline-ai/chronon#160
File: frontend/src/routes/joins/[slug]/services/joins.service.ts:34-34
Timestamp: 2025-01-17T00:33:14.792Z
Learning: The join timeseries API in `frontend/src/routes/joins/[slug]/services/joins.service.ts` specifically requires 'drift' as the metric type, regardless of the metricType parameter passed to the function.
frontend/src/routes/joins/[slug]/services/joins.service.ts (1)
Learnt from: ken-zlai
PR: zipline-ai/chronon#160
File: frontend/src/routes/joins/[slug]/services/joins.service.ts:34-34
Timestamp: 2025-01-17T00:33:14.792Z
Learning: The join timeseries API in `frontend/src/routes/joins/[slug]/services/joins.service.ts` specifically requires 'drift' as the metric type, regardless of the metricType parameter passed to the function.
🔇 Additional comments (53)
frontend/src/lib/components/charts/PercentileLineChart.svelte (4)
7-10: Imports look fine.
All imports appear valid. Confirm that external dependencies are installed in the build environment.
16-16: New data type usage looks consistent.
Ensure all callers of this component provide the correct structure for ITileSummarySeries.
31-33: Validate array bounds when accessing percentiles.
Ensure c.index is within percentiles length.
36-41: Null check is correct.
Converting NULL_VALUE to null ensures expected chart rendering.frontend/src/routes/joins/[slug]/services/joins.service.ts (11)
2-2: Imports look correct.
8-8: Matches new drift interface.
10-10: Signature aligns with drift usage.
22-22:driftMetricparameter is consistent.
24-27: Check for undefinedmodels.models.
30-33: Fallback tracking is fine.
35-41: Consider error handling on API call.
43-44: Empty check ensures fallback usage.
45-50: Date range fallback is clear.
53-58: Repeated API call also needs error handling.
61-66: Return shape matches new structure.frontend/src/lib/components/charts/FeaturesLineChart.svelte (8)
4-5: New imports are valid.
8-8: Adopts drift format.
18-18: Props updated for drift.
30-31: Extracting columns is neat.
38-41: Handles missing arrays gracefully.
43-44: Using column as key is consistent.
46-47: NULL_VALUE check is correct.
50-50: Applying color scale is good.frontend/src/routes/joins/[slug]/observability/drift/+page.svelte (22)
5-6: Imports for utils and d3 are valid.
11-16: New drift metric utilities.
32-33: Sort helpers and NULL const.
41-46: Sorting context set to 'drift'.
47-47: Sort direction derived from params.
49-49: Baseline offset is a week.
52-62: Groups drift series by groupName, sorted.
64-67: Domain derived from DRIFT_METRIC_SCALES.
83-84: Collapsible states bound to group names.
86-87: Stores column summary data.
94-99:tryblock used for column series.
100-111: Parallel calls fetch summary data.
114-115: Assigning API results to local states.
119-120: Fallback to null on error.
161-162: Margins for chart spacing.
165-166: Passing drift data and domain to chart.
225-228: Finding drift group from selected column.
234-235: Chart uses same domain in dialog.
253-254: Checks for percentile summary.
268-290: Histogram logic uses timestamp indexing.
300-301: Baseline data assigned properly.
305-306: Current data assigned properly.frontend/src/lib/components/DriftMetricToggle.svelte (2)
9-12: LGTM! Good use of queryParameters configuration.The configuration prevents unnecessary history updates and default value display.
15-26: LGTM! Clean implementation of the toggle buttons.Good use of Svelte's each block and conditional styling.
frontend/src/lib/util/drift-metric.ts (2)
19-27: LGTM! Good type safety implementation.The use of
satisfiesoperator ensures type safety while maintaining good inference.
13-17: Verify drift metric scale values.The max value of 25 for PSI seems arbitrary. Please verify if this is the correct upper bound.
frontend/src/routes/joins/[slug]/+layout.svelte (1)
16-16: Consider adding error boundary for drift data.The title relies on deeply nested optional chaining. Consider adding error boundaries to handle cases where drift data is unavailable.
frontend/src/lib/components/ChartControls.svelte (1)
3-3: LGTM!Clean replacement of MetricTypeToggle with DriftMetricToggle.
Also applies to: 49-49
frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte (1)
75-81: LGTM!Clean update to use new ITileSummarySeries type.
frontend/src/lib/api/api.ts (1)
106-106: LGTM!Clean update to use IJoinDriftResponse type.
Also applies to: 123-125
…Response` type for `getJoinDrift`/`getColumnDrif`
…aturesLineChart. Simplify data loading and hande empty results (no longer 500). Rename `MetricType` with `DriftMetric` (use and align to Thrift interface), handling type change (enum/number vs string). Client side sort
…ries` instead of `histogramDriftSeries`, aligning with `TimeSeriesHandler` `isNumeric`)
…e `PercentileLineChart` to new `getColumnSummary` / `ITileSummarySeries` data structure
…nd null ratio PieChart to use new API endpoint data structures
…o not returning null
sean-zlai
force-pushed
the
sean/thrift-observability-apis
branch
from
1dfdd63 to
1927b04
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
frontend/src/lib/components/charts/PercentileLineChart.svelte (1)
27-29: Extract hardcoded percentile indices into constants.Move magic numbers (19, 10, 1) to named constants for better maintainability.
+const PERCENTILE_INDICES = { + P95: 19, + P50: 10, + P5: 1 +}; + series={[ - { label: 'p95', color: '#4B92FF', index: 19 }, - { label: 'p50', color: '#7DFFB3', index: 10 }, - { label: 'p5', color: '#FDDD61', index: 1 } + { label: 'p95', color: '#4B92FF', index: PERCENTILE_INDICES.P95 }, + { label: 'p50', color: '#7DFFB3', index: PERCENTILE_INDICES.P50 }, + { label: 'p5', color: '#FDDD61', index: PERCENTILE_INDICES.P5 }frontend/src/routes/joins/[slug]/observability/drift/+page.svelte (2)
41-62: Add error handling for data processing.Consider adding error handling for cases where
data.joinDrift.driftSeriesmight be undefined.const driftSeriesByGroupName = $derived( + data.joinDrift?.driftSeries ? sort( rollups( data.joinDrift.driftSeries, (values) => sort(values, (d) => d.key.column, sortDirection), (d) => d.key?.groupName ?? 'Unknown' ), (d) => d[0], sortDirection ) + : [] );
270-272: Improve type safety by updating type definitions.Multiple
as unknown as numbercasts suggest type definition issues. Consider updatingITileSummarySeriestype to avoid unsafe type casting.Also applies to: 328-331
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (22)
frontend/src/lib/api/api.ts(3 hunks)frontend/src/lib/components/ChartControls.svelte(2 hunks)frontend/src/lib/components/DriftMetricToggle.svelte(1 hunks)frontend/src/lib/components/MetricTypeToggle.svelte(0 hunks)frontend/src/lib/components/charts/FeaturesLineChart.svelte(3 hunks)frontend/src/lib/components/charts/PercentileLineChart.svelte(2 hunks)frontend/src/lib/types/MetricType/MetricType.ts(0 hunks)frontend/src/lib/types/Model/Model.ts(0 hunks)frontend/src/lib/util/drift-metric.ts(1 hunks)frontend/src/lib/util/sample-data.ts(0 hunks)frontend/src/lib/util/sort.ts(0 hunks)frontend/src/routes/+layout.ts(1 hunks)frontend/src/routes/joins/[slug]/+layout.svelte(1 hunks)frontend/src/routes/joins/[slug]/+layout.ts(1 hunks)frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte(4 hunks)frontend/src/routes/joins/[slug]/observability/distributions/+page.ts(1 hunks)frontend/src/routes/joins/[slug]/observability/drift/+page.svelte(9 hunks)frontend/src/routes/joins/[slug]/services/joins.service.ts(1 hunks)frontend/src/routes/thrift/+page.svelte(0 hunks)hub/src/main/java/ai/chronon/hub/HubVerticle.java(0 hunks)hub/src/main/scala/ai/chronon/hub/handlers/TimeSeriesHandler.scala(0 hunks)hub/src/test/scala/ai/chronon/hub/handlers/TimeSeriesHandlerTest.scala(0 hunks)
💤 Files with no reviewable changes (9)
- frontend/src/lib/components/MetricTypeToggle.svelte
- frontend/src/lib/util/sort.ts
- hub/src/test/scala/ai/chronon/hub/handlers/TimeSeriesHandlerTest.scala
- hub/src/main/scala/ai/chronon/hub/handlers/TimeSeriesHandler.scala
- frontend/src/routes/thrift/+page.svelte
- frontend/src/lib/types/MetricType/MetricType.ts
- hub/src/main/java/ai/chronon/hub/HubVerticle.java
- frontend/src/lib/util/sample-data.ts
- frontend/src/lib/types/Model/Model.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- frontend/src/routes/+layout.ts
- frontend/src/routes/joins/[slug]/+layout.svelte
- frontend/src/lib/components/ChartControls.svelte
- frontend/src/routes/joins/[slug]/+layout.ts
- frontend/src/lib/components/DriftMetricToggle.svelte
- frontend/src/routes/joins/[slug]/observability/distributions/+page.svelte
- frontend/src/lib/util/drift-metric.ts
🧰 Additional context used
📓 Learnings (1)
frontend/src/routes/joins/[slug]/services/joins.service.ts (1)
Learnt from: ken-zlai
PR: zipline-ai/chronon#160
File: frontend/src/routes/joins/[slug]/services/joins.service.ts:34-34
Timestamp: 2025-01-17T00:33:14.792Z
Learning: The join timeseries API in `frontend/src/routes/joins/[slug]/services/joins.service.ts` specifically requires 'drift' as the metric type, regardless of the metricType parameter passed to the function.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: frontend_tests
- GitHub Check: non_spark_tests
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (7)
frontend/src/routes/joins/[slug]/observability/distributions/+page.ts (2)
11-11: Address TODO comment about error handling.Consider showing errored columns in an error state instead of ignoring them.
Would you like me to propose an implementation for error state handling?
7-7: Improve null safety for joinName extraction.The fallback to 'Unknown' might mask issues.
frontend/src/lib/components/charts/FeaturesLineChart.svelte (1)
40-41: LGTM! Clear documentation of data types.The comment explaining different series types for numeric vs categorical columns is helpful.
frontend/src/lib/api/api.ts (3)
9-10: LGTM!Type imports align with the shift from timeseries to drift-based approach.
106-106: LGTM!Return type update aligns with the new drift-based approach.
123-125: LGTM!Return type update aligns with the new drift-based approach.
frontend/src/routes/joins/[slug]/observability/drift/+page.svelte (1)
5-8: LGTM!Imports support the new drift-based data processing approach.
Also applies to: 16-16, 32-33
| isUsingFallback: true | ||
| } | ||
| // If empty data is results, use fallback date range | ||
| const useFallback = joinDrift.driftSeries.every((ds) => Object.keys(ds).length <= 1); // Only `key` returned on results |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve empty data check robustness.
Current check assumes only key property indicates empty data. Consider a more explicit check.
-const useFallback = joinDrift.driftSeries.every((ds) => Object.keys(ds).length <= 1);
+const useFallback = joinDrift.driftSeries.every((ds) =>
+ !ds.percentileDriftSeries?.length && !ds.histogramDriftSeries?.length
+);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const useFallback = joinDrift.driftSeries.every((ds) => Object.keys(ds).length <= 1); // Only `key` returned on results | |
| const useFallback = joinDrift.driftSeries.every((ds) => | |
| !ds.percentileDriftSeries?.length && !ds.histogramDriftSeries?.length | |
| ); |
| <!-- TODO: Remove mockNullValue once data is populated --> | ||
| {@const mockNullValue = Math.random() * 100} | ||
| <div class="grid gap-2"> | ||
| <div class="h-[230px]"> | ||
| <!-- TODO: colors copied from ECharts defaults --> | ||
| <PieChart | ||
| data={[ | ||
| { | ||
| label: 'Null Value Percentage', | ||
| // value: c.value | ||
| value: mockNullValue | ||
| }, | ||
| { | ||
| label: 'Non-null Value Percentage', | ||
| // value: 100 - c.value | ||
| value: 100 - mockNullValue | ||
| } |
There was a problem hiding this comment.
Remove mock values.
Replace mock null values with actual data from the API.
-{@const mockNullValue = Math.random() * 100}
<PieChart
data={[
{
label: 'Null Value Percentage',
- // value: c.value
- value: mockNullValue
+ value: c.value
},
{
label: 'Non-null Value Percentage',
- // value: 100 - c.value
- value: 100 - mockNullValue
+ value: 100 - c.value
}
]}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <!-- TODO: Remove mockNullValue once data is populated --> | |
| {@const mockNullValue = Math.random() * 100} | |
| <div class="grid gap-2"> | |
| <div class="h-[230px]"> | |
| <!-- TODO: colors copied from ECharts defaults --> | |
| <PieChart | |
| data={[ | |
| { | |
| label: 'Null Value Percentage', | |
| // value: c.value | |
| value: mockNullValue | |
| }, | |
| { | |
| label: 'Non-null Value Percentage', | |
| // value: 100 - c.value | |
| value: 100 - mockNullValue | |
| } | |
| <!-- TODO: Remove mockNullValue once data is populated --> | |
| <div class="grid gap-2"> | |
| <div class="h-[230px]"> | |
| <!-- TODO: colors copied from ECharts defaults --> | |
| <PieChart | |
| data={[ | |
| { | |
| label: 'Null Value Percentage', | |
| value: c.value | |
| }, | |
| { | |
| label: 'Non-null Value Percentage', | |
| value: 100 - c.value | |
| }, | |
| ]} | |
| /> |
| // TODO: Add loading and error states | ||
| const [_columnSummaryData, _columnSummaryBaselineData] = await Promise.all([ | ||
| api.getColumnSummary({ | ||
| name: joinName, | ||
| columnName: columnName, | ||
| startTs: data.dateRange.startTimestamp, | ||
| endTs: data.dateRange.endTimestamp, | ||
| granularity: 'percentile', | ||
| metricType: 'drift', | ||
| metrics: 'value', | ||
| offset: '1D', | ||
| algorithm: 'psi' | ||
| endTs: data.dateRange.endTimestamp | ||
| }), | ||
| api.getFeatureTimeseries({ | ||
| joinId: data.joinTimeseries.name, | ||
| featureName, | ||
| startTs: data.dateRange.startTimestamp, | ||
| endTs: data.dateRange.endTimestamp, | ||
| metricType: 'drift', | ||
| metrics: 'null', | ||
| offset: '1D', | ||
| algorithm: 'psi', | ||
| granularity: 'percentile' | ||
| api.getColumnSummary({ | ||
| name: joinName, | ||
| columnName: columnName, | ||
| startTs: Number(sub(new Date(data.dateRange.startTimestamp), baselineOffset)), | ||
| endTs: Number(sub(new Date(data.dateRange.endTimestamp), baselineOffset)) | ||
| }) | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement loading and error states.
The TODO comment indicates missing loading and error states. Consider implementing them for better UX.
+let isLoading = $state(false);
+let error = $state<Error | null>(null);
async function selectSeriesPoint(seriesPoint: typeof selectedSeriesPoint) {
selectedSeriesPoint = seriesPoint;
+ isLoading = true;
+ error = null;
if (seriesPoint) {
try {
// ... existing code ...
} catch (err) {
- console.error('Error fetching data:', error);
+ error = err instanceof Error ? err : new Error('Unknown error');
columnSummaryData = null;
columnSummaryBaselineData = null;
+ } finally {
+ isLoading = false;
}
}
}Committable suggestion skipped: line range outside the PR's diff.
ken-zlai
left a comment
ken-zlai
left a comment
There was a problem hiding this comment.
great stuff here! after the changes we discussed async this is good to go
Summary
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor