observability: new page based on dataplane observability API

[nicolaferraro]

When the feature flag is toggled, console starts showing this and cloud-ui stops showing the old one (different ff for Serverless and Serverful).

This is a first iteration, it allows to select the time range (but only from now back to 6 hours). I need to publish the max lookback time in the API to have a better control bar.

[malinskibeniamin]

Let's use recharts v3 shadcn-ui/ui#7669 which will require components to be upgraded.

We can do that in a followup PR

[malinskibeniamin]

will this be required for standalone mode

[sago2k8]

@claude could you review, focus on simplicity and ensure it follows some well known frontend best practices

[malinskibeniamin]

this component will require updating to support shadcn chart + recharts v3 major release.

[malinskibeniamin]

@weeco implemented this for transcripts/traces UI, can you please adapt a similar approach to follow the same practices?

[weeco]

https://github.com/redpanda-data/console/pull/2202/changes#diff-3d39ec19551b863b267e002b020aa0c055b390f7f706b8c24147427b3c111222R83-R92

We can move some things to the utils directories.

[malinskibeniamin]

do we still need useExhaustiveDependencies?

[weeco]

In addition to this we also need to make sure that this is actually configured in the backend I'd say - specifically thinking of self-hosted community users and customers here. Not sure if we want to disable/hide the sidebar or show a setup / onboarding page in this case.

[malinskibeniamin]

Please use Redpanda UI registry instead of @redpanda-data/ui for this new observability page.

[malinskibeniamin]

I think we should filter using storage utils

[malinskibeniamin]

blocker: use redpanda UI registry

[malinskibeniamin]

@claude could you review, focus on simplicity and ensure it follows some well known frontend best practices

@sago2k8 it's not enabled yet AFAIK

[nicolaferraro]

I did a first pass to remove the old components and fixing stuff. About the self-hosted mode, currently the sidebar is only shown for embedded mode only. Don't know if there's a clean way to make this work for self-hosted as well

[malinskibeniamin]

@claude review

[claude]

Claude finished @malinskibeniamin's task in 2m 36s —— View job

---

Code Review

I've reviewed the observability page implementation focusing on simplicity and frontend best practices. Here's my comprehensive feedback:

---

Summary

Overall, the implementation is well-structured with good test coverage and proper TypeScript typing. However, there are several important issues to address, particularly regarding code consistency with existing patterns and some potential bugs.

---

Critical Issues (Must Fix)

1. Biome Linting Violations

The github-actions bot has flagged multiple formatting issues that should be addressed. These are primarily spacing/formatting issues in:

• metric-chart.tsx: Lines 18, 20, 130, 138, 149, 153, 163
• observability-page.tsx: Lines 19, 22, 83

Fix linting issues →

2. Inconsistent Import Paths

metric-chart.tsx:20 - The import statement doesn't follow the pattern used elsewhere:

import { prettyNumber } from 'utils/utils';

Should be relative like other imports in the file:

import { prettyNumber } from '../../../utils/utils';

This is inconsistent with the other imports in the same file which use relative paths like ../../redpanda-ui/components/.

3. useExhaustiveDependencies Pattern

Both observability-page.tsx:52 and observability-toolbar.tsx:44 use biome-ignore comments for useExhaustiveDependencies. According to the existing review comment from @malinskibeniamin, this pattern should be reconsidered. The refreshKey dependency pattern is valid, but the implementation could be cleaner:

Current approach (observability-page.tsx:42-53):

const refreshData = useCallback(() => {
  setRefreshKey((prev) => prev + 1);
  refetch();
}, [refetch]);

// biome-ignore lint/correctness/useExhaustiveDependencies: refreshKey triggers recalculation on refresh
const timeRange = useMemo(() => calculateTimeRange(selectedTimeRange), [selectedTimeRange, refreshKey]);

Suggested approach: Consider using a timestamp or Date object instead of an incrementing key for better clarity:

const [lastRefreshTime, setLastRefreshTime] = useState(Date.now());

const refreshData = useCallback(() => {
  setLastRefreshTime(Date.now());
  refetch();
}, [refetch]);

const timeRange = useMemo(
  () => calculateTimeRange(selectedTimeRange), 
  [selectedTimeRange, lastRefreshTime]
);

This makes the intent clearer and avoids the biome-ignore.

---

High Priority Issues

4. Code Duplication in Time Range Calculation

The time range calculation logic is duplicated between observability-page.tsx and observability-toolbar.tsx:

• observability-page.tsx:53 uses calculateTimeRange(selectedTimeRange)
• observability-toolbar.tsx:45-53 reimplements the same logic inline

The calculateTimeRange function exists and is exported (observability-toolbar.tsx:111-119), but the toolbar component doesn't use it. This is unnecessary duplication and a maintenance risk.

Fix: Remove the inline calculation in ObservabilityToolbar and use the exported function:

// Current (lines 45-53):
const timeRange = useMemo(() => {
  const now = new Date();
  const startTime = new Date(now.getTime() - TIME_RANGE_MS[selectedTimeRange]);
  return { start: startTime, end: now };
}, [selectedTimeRange, refreshKey]);

// Should be:
const timeRange = useMemo(
  () => calculateTimeRange(selectedTimeRange), 
  [selectedTimeRange, refreshKey]
);

5. Grid Layout Classes

observability-page.tsx:83 - The grid classes appear correct but differ from the Biome suggestion. Current:

<div className="grid gap-6 grid-cols-1 lg:grid-cols-2">

Biome suggests:

<div className="grid grid-cols-1 gap-6 lg:grid-cols-2">

The suggested order follows Tailwind's recommended class ordering (layout → spacing → responsive).

6. Error Handling Could Be More Specific

metric-chart.tsx:135-142 - The error state treats all errors the same way. Consider differentiating between:

• Network errors (suggest retry)
• 404 (metric not found)
• Permission errors
• Timeout errors

This would provide better UX and debugging information.

---

Medium Priority Issues

7. Magic Numbers in Color Array

metric-chart.tsx:109-115 - The chart colors are hardcoded. Consider:

// Current:
const colors = [
  'var(--color-chart-1)',
  'var(--color-chart-2)',
  // ...
];

// Better: Define as constant outside component
const CHART_COLORS = [
  'var(--color-chart-1)',
  'var(--color-chart-2)',
  'var(--color-chart-3)',
  'var(--color-chart-4)',
  'var(--color-chart-5)',
] as const;

This prevents recreation on every render and makes the colors reusable.

8. Potential NaN Timestamp

metric-chart.tsx:237-244 - Multiple checks for NaN timestamp, but the flow could be simplified:

const timestamp = payload?.[0]?.payload?.timestamp;
if (!timestamp || typeof timestamp !== 'number') return '';
const date = new Date(timestamp);
if (!date.getTime()) return ''; // getTime() returns 0 for invalid dates

9. Timezone Hardcoded to UTC

observability-toolbar.tsx:104 and metric-chart.tsx:179, 250 - UTC is hardcoded. Consider:

• Making timezone configurable via user preferences
• Or at minimum, document why UTC is required (e.g., backend always returns UTC)

10. Type Safety Could Be Improved

metric-chart.tsx:40-60 - The addDataPoint function accepts point with optional properties but doesn't validate the structure deeply. Consider using a more specific type or adding runtime validation.

---

Low Priority / Nice to Have

11. Chart Height Hardcoded

metric-chart.tsx:168 - Height is hardcoded to 250px. Consider making this configurable via props if different charts need different heights in the future.

12. Loading State Could Show Skeleton Count

observability-page.tsx:55-62 - Shows 3 skeleton cards regardless of how many charts will actually load. Consider showing the correct number if queries is available from cache.

13. Test Coverage Gaps

observability-page.test.tsx - Tests are good but missing:

• Test for the refresh functionality (appGlobal.onRefresh)
• Test for time range changes
• Test for toolbar interactions

14. Accessibility Improvements

• observability-toolbar.tsx:73-108 - The toolbar could benefit from ARIA labels
• metric-chart.tsx - Consider adding aria-label to charts for screen readers
• The refresh button uses global state but should ideally have keyboard support

---

Performance Observations

✅ Good:

• Proper use of useMemo to prevent unnecessary recalculations
• useCallback for event handlers
• Chart data transformation is memoized

⚠️ Could be improved:

• observability-page.tsx:84-86 - The .map() creates new <MetricChart> components on every render. Consider adding React.memo() to MetricChart if queries list is large.

---

Code Style & Consistency

✅ Good:

• Consistent TypeScript typing throughout
• Good separation of concerns (separate files for chart, toolbar, page)
• Proper copyright headers
• Clean component structure

⚠️ Inconsistencies:

• Import path styles (relative vs absolute) - see issue Add prometheus metrics #2
• Some components use functional declarations while others might benefit from consistency

---

Security

✅ No security issues identified. The code:

• Uses proper protobuf message creation
• Doesn't execute arbitrary code
• Doesn't expose sensitive data
• Uses proper XSS protection (React's built-in escaping)

---

Testing

✅ Good test coverage with proper mocking and protobuf usage. Tests cover:

• Empty state
• Loading state
• Error state
• Success with data

---

Addressing Existing Review Comments

From previous reviewers:

1. @malinskibeniamin: "use redpanda UI registry" ✅ - All components correctly use ../../redpanda-ui/components/ imports. No @redpanda-data/ui imports found.

2. @malinskibeniamin: "filter using storage utils" ⚠️ - I couldn't find storage utils in the codebase. If this refers to persisting filter state, that's not currently implemented.

3. @malinskibeniamin: "useExhaustiveDependencies" - Addressed in issue Create helm chart #3 above.

4. @weeco: "similar approach to transcripts/traces UI" - The current implementation does follow a similar pattern with toolbar + data grid layout.

5. @weeco: "self-hosted mode" - The route guard (routes/observability/index.tsx:23-33) only checks feature flags. No backend configuration check is present. This could lead to showing an empty page if the backend doesn't support the API.

---

Recommendations Priority

Must fix before merge:

1. Fix all Biome linting issues (automated suggestions available)
2. Fix inconsistent import path (issue Add prometheus metrics #2)
3. Remove duplicated time range calculation (issue Add dark theme  #4)

Should fix before merge:
4. Consider alternative to useExhaustiveDependencies pattern (issue #3)
5. Add backend configuration check for self-hosted mode (issue from @weeco)
6. Fix grid class ordering (issue #5)

Can be addressed in follow-up:
7. Improve error handling specificity
8. Extract color constants
9. Make timezone configurable
10. Add missing test coverage
11. Performance optimization with React.memo()

---

Conclusion

This is a solid first iteration of the observability page. The core functionality is well-implemented with good separation of concerns and test coverage. The main areas for improvement are:

1. Code consistency - Import paths and React patterns should be uniform
2. Simplification - Remove code duplication in time range calculations
3. Robustness - Add backend configuration checks for self-hosted scenarios
4. Polish - Address linting issues and class ordering

The implementation follows good React practices overall and the component architecture is clean. Once the critical issues are addressed, this will be ready to merge.

---

[malinskibeniamin]

let's use storage utils to get proper formatting for these units

[malinskibeniamin]

This should be gated by isAdpEnabled() too, right? Or will we have observability in self-hosted too 1 day, somehow?

[nicolaferraro]

Technically it's possible to provide it in self-hosted, by configuring a local prom server. There's an internal discussion about it. For the moment, I think this condition only enables it in embedded mode with a specific feature flag.

[weeco]

I don't see why this should be gated by isAdpEnabled(). This change is completely unrelated. Current condition looks fine to me, even though I'd personally rather do the backend check to see if the API is supported rather than relying on the feature flags that much.

[malinskibeniamin]

Do we want to poll this?

[nicolaferraro]

I did not wire auto-polling since the page where charts are used has an auto-refresh button, that also moves the clock forward.

Polling without applying a time shift does nothing (the API uses an absolute time range).

[malinskibeniamin]

can we rely on work from @weeco for transcripts? and use the same approach for the time ranges

[nicolaferraro]

Done another pass, I hope I got what you meant :)

[weeco]

@claude Review this and check if the feedback from reviewers has been applied or if someone was missing (in case it was reasonable feedback).

[claude]

Claude finished @weeco's task in 2m 38s —— View job

---

Code Review: Observability Page Implementation

I've reviewed the PR to check if previous reviewer feedback has been properly addressed. Here's my comprehensive analysis:

---

Summary

The author has done an excellent job addressing most of the previous review feedback. The implementation now follows best practices with proper use of the Redpanda UI registry, shared time-range utilities with transcripts, and well-structured unit formatting. However, there are a few remaining concerns that should be addressed.

---

✅ Feedback Successfully Addressed

1. Use Redpanda UI Registry ✅

Status: RESOLVED

All components now correctly use the Redpanda UI registry pattern:

• frontend/src/components/redpanda-ui/components/chart.tsx has been added
• All imports use ../../redpanda-ui/components/* pattern
• No @redpanda-data/ui imports found in observability code

2. Follow Transcripts/Traces UI Approach ✅

Status: RESOLVED

The implementation now follows the same time-range pattern used in transcripts:

• Shared frontend/src/utils/time-range.ts utility (commit 271f920)
• Both transcripts and observability use getTimeRanges() and calculateTimeRange()
• Consistent toolbar design with time range selectors

3. Unit Formatting ✅

Status: RESOLVED

Proper unit formatting is now implemented:

• New frontend/src/utils/unit.ts with formatWithUnit() function
• Comprehensive unit abbreviations (bytes, time, compound units)
• Applied in Y-axis and tooltip formatting (metric-chart.tsx:136, 146)
• Well-tested with 56 lines of unit tests

4. Code Organization ✅

Status: RESOLVED

Code has been properly organized into utilities:

• frontend/src/utils/time-range.ts - Time range calculations
• frontend/src/utils/unit.ts - Unit formatting
• frontend/src/components/pages/observability/utils/chart-data.ts - Chart data transformation
• Clean separation of concerns

5. UseExhaustiveDependencies Pattern ✅

Status: ACCEPTABLE

The biome-ignore comments remain on:

• observability-page.tsx:53
• observability-toolbar.tsx:43

However, this is now acceptable because:

• The refreshKey pattern is legitimate for triggering recalculations
• Both usages have clear explanatory comments
• The pattern is intentional and not a mistake

---

⚠️ Issues Requiring Attention

1. Missing isAdpEnabled() Check in Route Visibility ⚠️

Priority: HIGH
File: frontend/src/utils/route-utils.tsx:198-204

Issue: The observability sidebar entry doesn't check isAdpEnabled() like other ADP features (transcripts, AI agents, knowledge bases).

Current Code:

{
  path: '/observability',
  title: 'Metrics',
  icon: ActivityIcon,
  visibilityCheck: routeVisibility(
    () =>
      isEmbedded() &&
      (isServerless()
        ? isFeatureFlagEnabled('enableDataplaneObservabilityServerless')
        : isFeatureFlagEnabled('enableDataplaneObservability'))
  ),
},

Should Be:

{
  path: '/observability',
  title: 'Metrics',
  icon: ActivityIcon,
  visibilityCheck: routeVisibility(
    () =>
      isEmbedded() &&
      isAdpEnabled() &&  // Add this check
      (isServerless()
        ? isFeatureFlagEnabled('enableDataplaneObservabilityServerless')
        : isFeatureFlagEnabled('enableDataplaneObservability'))
  ),
},

Why it matters: Without this check, the sidebar might show the Metrics link even when ADP is not configured in self-hosted deployments, leading to a confusing user experience.

Comparison with other ADP features:

• Transcripts: isEmbedded() && isAdpEnabled() && ... (line 276)
• AI Agents: isEmbedded() && isAdpEnabled() && ... (line 315)
• Knowledge Bases: isAdpEnabled() && ... (line 235)

Fix missing isAdpEnabled check →

2. No Backend Configuration Check ⚠️

Priority: MEDIUM
File: frontend/src/routes/observability/index.tsx:23-33

Issue: As @weeco pointed out in the review comments, the route only checks feature flags but doesn't verify if the backend actually supports the observability API.

Context from @nicolaferraro's response:

"Technically it's possible to provide it in self-hosted, by configuring a local prom server. There's an internal discussion about it."

Current behavior:

• Feature flag enabled → page shows
• If backend doesn't support API → queries fail → error messages shown
• Not ideal UX for self-hosted users

Recommendation: Consider one of these approaches:

1. Add a backend capability check (similar to how other features check Feature.TracingService)
2. Show an onboarding/setup page when API is unavailable
3. Hide the sidebar entry entirely when backend doesn't support it

This can potentially be addressed in a follow-up PR if there's ongoing discussion about backend detection.

3. No Polling Configuration ℹ️

Priority: LOW
File: frontend/src/react-query/api/observability.ts:37

Review comment from @malinskibeniamin: "Do we want to poll this?"

Author's response (@nicolaferraro):

"I did not wire auto-polling since the page where charts are used has an auto-refresh button, that also moves the clock forward. Polling without applying a time shift does nothing (the API uses an absolute time range)."

Status: This is a valid design decision. The refresh button approach makes sense for time-range-based queries where polling wouldn't show new data without advancing the time window.

---

💡 Minor Observations

4. Recharts Version ℹ️

File: frontend/package.json:123

The PR uses recharts 2.15.4 instead of v3. @malinskibeniamin noted:

"Let's use recharts v3 which will require components to be upgraded. We can do that in a followup PR"

Status: Acknowledged as follow-up work. This is acceptable for this PR.

5. Grid Class Ordering ℹ️

File: observability-page.tsx:84

Current: className="grid grid-cols-1 gap-6 lg:grid-cols-2"

This follows Tailwind's recommended class ordering and is correct.

6. Chart Colors Constant ✅

File: chart-data.ts:14-20

Colors are now properly defined as a constant outside the component. Well done!

---

📊 Code Quality Assessment

Strengths:

• ✅ Clean component architecture with good separation of concerns
• ✅ Comprehensive test coverage (179 lines of tests)
• ✅ Proper TypeScript typing throughout
• ✅ Good use of React patterns (useMemo, useCallback)
• ✅ Consistent with existing codebase patterns
• ✅ Well-documented utility functions
• ✅ Proper error handling and loading states

Code Consistency:

• ✅ All imports now use relative paths correctly
• ✅ Follows Redpanda UI registry pattern
• ✅ Time-range utilities shared with transcripts feature
• ✅ Proper copyright headers

Testing:

• ✅ Unit tests for utilities (unit.test.ts - 56 lines)
• ✅ Integration tests for page component (179 lines)
• ✅ Tests cover loading, error, empty, and success states
• ⚠️ Missing: Tests for refresh functionality and time-range changes (acceptable for first iteration)

---

📋 Reviewer Feedback Status

Reviewer	Feedback	Status
@malinskibeniamin	Use recharts v3	✅ Acknowledged as follow-up
@malinskibeniamin	Required for standalone mode?	✅ Addressed with feature flag check
@malinskibeniamin	Use Redpanda UI registry	✅ RESOLVED
@malinskibeniamin	Filter using storage utils	❓ Unclear - no filtering implemented yet
@malinskibeniamin	useExhaustiveDependencies	✅ Acceptable pattern
@malinskibeniamin	Use similar approach to transcripts	✅ RESOLVED - shared time-range utils
@malinskibeniamin	Use storage utils for unit formatting	✅ RESOLVED - custom unit.ts created
@malinskibeniamin	Gate by isAdpEnabled()	⚠️ MISSING - should be added
@malinskibeniamin	Polling?	✅ Intentionally not implemented
@weeco	Backend configuration check	⚠️ Still missing
@weeco	Similar approach to transcripts	✅ RESOLVED
Previous Claude review	All linting issues	✅ Assumed resolved
Previous Claude review	Import path consistency	✅ RESOLVED
Previous Claude review	Code duplication	✅ RESOLVED

---

🎯 Recommendations

Must Fix Before Merge:

1. ⚠️ Add isAdpEnabled() check to route visibility in route-utils.tsx:200

Should Consider:
2. Add backend capability check or onboarding page for unsupported backends (can be follow-up)

Nice to Have (Follow-up PRs):
3. Upgrade to recharts v3
4. Add tests for refresh and time-range change functionality
5. Consider making timezone configurable (currently hardcoded to UTC)

---

✨ Conclusion

This PR shows significant improvement from the previous review. The author has successfully addressed the majority of reviewer feedback, particularly around:

• Using Redpanda UI registry consistently
• Sharing time-range utilities with transcripts
• Implementing proper unit formatting
• Code organization and structure

The implementation is well-structured, tested, and follows React best practices. The main outstanding issue is the missing isAdpEnabled() check for route visibility, which should be added for consistency with other ADP features.

Overall: Excellent work addressing previous feedback! Just needs the isAdpEnabled() check added, and this will be ready to merge.

---