feat(applaunchpad): support metrics-sdk#6720
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request introduces support for the sealos-metrics-sdk package in the applaunchpad frontend provider, replacing direct HTTP calls to monitoring endpoints with a centralized SDK-based approach. The change modernizes the monitoring infrastructure by providing type-safe interfaces and centralized authentication handling through the new metrics SDK.
Changes:
- Refactored monitoring data fetching to use
sealos-metrics-sdkinstead of direct API calls - Updated configuration from
monitortometricswith additionalwhitelistKubernetesHostsparameter - Simplified and consolidated adapter functions in
getMonitorData.tsby removing code duplication - Added workspace dependency on
sealos-metrics-sdkpackage
Reviewed changes
Copilot reviewed 6 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/providers/applaunchpad/src/types/monitor.d.ts | Removed unused MonitorServiceResult interface and BoxProps import (cleaned up 38 lines) |
| frontend/providers/applaunchpad/src/types/index.d.ts | Updated config type from monitor to metrics with whitelistKubernetesHosts array |
| frontend/providers/applaunchpad/src/services/monitorFetch.ts | Completely refactored to use MetricsClient SDK instead of direct fetch API |
| frontend/providers/applaunchpad/src/pages/api/monitor/getMonitorData.ts | Refactored adapter functions, added query type mapping, improved parameter normalization |
| frontend/providers/applaunchpad/src/instrumentation.ts | Added metrics configuration to default config alongside existing monitor config |
| frontend/providers/applaunchpad/package.json | Added sealos-metrics-sdk workspace dependency |
| frontend/providers/applaunchpad/deploy/manifests/deploy.yaml.tmpl | Updated deployment config to use metrics instead of monitor with default URL |
| frontend/providers/applaunchpad/data/config.yaml | Updated development config to use new metrics URL and configuration structure |
| frontend/pnpm-lock.yaml | Added workspace link for sealos-metrics-sdk package |
Files not reviewed (1)
- frontend/pnpm-lock.yaml: Language not supported
| } | ||
| throw error; | ||
| } | ||
| return client.launchpad.query(params); |
There was a problem hiding this comment.
The new monitorFetch function removes all explicit error handling that was present in the original implementation. The original code had a try-catch block with environment-specific error messages. While the SDK may handle errors internally, consider wrapping the SDK call in a try-catch block to provide more context-specific error messages for debugging, especially during development. This will help identify whether issues are with the SDK configuration, network connectivity, or authentication.
| return client.launchpad.query(params); | |
| try { | |
| return await client.launchpad.query(params); | |
| } catch (error) { | |
| if (process.env.NODE_ENV !== 'production') { | |
| // Provide more context during development to help debug SDK, network, or auth issues | |
| // Do not change the error shape; just add logging. | |
| // eslint-disable-next-line no-console | |
| console.error('monitorFetch: failed to query launchpad metrics', { | |
| error, | |
| metricsURL: metricsConfig?.url | |
| }); | |
| } | |
| throw error; | |
| } |
| data.data.result.map((item) => ({ | ||
| name: item.metric[metricName] || item.metric.pod || item.metric.persistentvolumeclaim, | ||
| xData: item.values?.map((value) => Number(value[0])) ?? [], | ||
| yData: item.values?.map((value) => Number.parseFloat(value[1]).toFixed(2)) ?? [] |
There was a problem hiding this comment.
The adaptChartData function uses Number.parseFloat(value[1]).toFixed(2) which returns a string. Since yData is defined as string[] in the MonitorDataResult type, this is technically correct. However, the original code used parseFloat(value[1]).toFixed(2) which has the same behavior. The change to Number.parseFloat is fine, but ensure this is intentional as it adds an extra level of namespace access without functional benefit.
| data.data.result.map((item) => ({ | ||
| name: item.metric[metricName] || item.metric.pod || item.metric.persistentvolumeclaim, | ||
| xData: item.values?.map((value) => Number(value[0])) ?? [], | ||
| yData: item.values?.map((value) => Number.parseFloat(value[1]).toFixed(2)) ?? [] | ||
| })); | ||
|
|
There was a problem hiding this comment.
The fallback logic in adaptChartData (line 22) uses item.metric[metricName] || item.metric.pod || item.metric.persistentvolumeclaim to ensure a name is always provided. This is good defensive programming. However, ensure that this fallback is tested, as it could mask issues where the expected metric name is missing. Consider logging a warning when falling back to avoid silent failures in production.
| data.data.result.map((item) => ({ | |
| name: item.metric[metricName] || item.metric.pod || item.metric.persistentvolumeclaim, | |
| xData: item.values?.map((value) => Number(value[0])) ?? [], | |
| yData: item.values?.map((value) => Number.parseFloat(value[1]).toFixed(2)) ?? [] | |
| })); | |
| data.data.result.map((item) => { | |
| const primaryName = item.metric[metricName]; | |
| const fallbackName = item.metric.pod || item.metric.persistentvolumeclaim; | |
| const name = primaryName || fallbackName || 'unknown'; | |
| if (!primaryName && fallbackName) { | |
| console.warn( | |
| `[monitor] adaptChartData: missing expected metric "${metricName}" in item.metric, using fallback name "${fallbackName}".` | |
| ); | |
| } else if (!primaryName && !fallbackName) { | |
| console.warn( | |
| `[monitor] adaptChartData: missing "${metricName}", "pod", and "persistentvolumeclaim" in item.metric; using "unknown" as name.` | |
| ); | |
| } | |
| return { | |
| name, | |
| xData: item.values?.map((value) => Number(value[0])) ?? [], | |
| yData: item.values?.map((value) => Number.parseFloat(value[1]).toFixed(2)) ?? [] | |
| }; | |
| }); |
| const queryTypeMap: Record<keyof MonitorQueryKey, LaunchpadMetricType> = { | ||
| cpu: 'cpu', | ||
| memory: 'memory', | ||
| disk: 'disk', | ||
| average_cpu: 'average_cpu', | ||
| average_memory: 'average_memory', | ||
| storage: 'disk' |
There was a problem hiding this comment.
The queryTypeMap maps storage to disk, but the SDK's LaunchpadMetricType may not distinguish between these two. Verify that the SDK correctly handles the disk type for storage queries, especially when a pvcName is provided. The mapping seems correct based on line 45 where both use disk, but ensure this is documented in the SDK.
| const normalizeQueryParam = (value: string | string[] | undefined) => | ||
| Array.isArray(value) ? value[0] : value; |
There was a problem hiding this comment.
The normalizeQueryParam function only handles string and string[] types but doesn't handle the case where the value is undefined. While TypeScript shows this is acceptable via the signature string | string[] | undefined, the function should explicitly handle undefined by returning it directly. Currently, if value is undefined, it will return undefined correctly due to the implicit else case, but this could be more explicit for better code clarity.
| const normalizeQueryParam = (value: string | string[] | undefined) => | |
| Array.isArray(value) ? value[0] : value; | |
| const normalizeQueryParam = (value: string | string[] | undefined) => { | |
| if (value === undefined) return value; | |
| return Array.isArray(value) ? value[0] : value; | |
| }; |
| const client = new MetricsClient({ | ||
| kubeconfig, | ||
| metricsURL: metricsConfig?.url, | ||
| whitelistKubernetesHosts: metricsConfig?.whitelistKubernetesHosts |
There was a problem hiding this comment.
The metricsConfig?.url uses optional chaining which is good for safety. However, if both metricsConfig?.url and metricsConfig?.whitelistKubernetesHosts are undefined, the SDK will use its default values. Consider validating that the metrics configuration is properly loaded at application startup to catch configuration errors early, rather than discovering them when the first monitoring request is made.
| const client = new MetricsClient({ | |
| kubeconfig, | |
| metricsURL: metricsConfig?.url, | |
| whitelistKubernetesHosts: metricsConfig?.whitelistKubernetesHosts | |
| if (!metricsConfig || (metricsConfig.url === undefined && metricsConfig.whitelistKubernetesHosts === undefined)) { | |
| throw new Error('Metrics configuration for launchpad is missing or invalid: expected metrics.url or metrics.whitelistKubernetesHosts to be defined.'); | |
| } | |
| const client = new MetricsClient({ | |
| kubeconfig, | |
| metricsURL: metricsConfig.url, | |
| whitelistKubernetesHosts: metricsConfig.whitelistKubernetesHosts |
| components: | ||
| monitor: | ||
| url: {{ .monitorUrl }} | ||
| metrics: |
There was a problem hiding this comment.
The deployment template uses {{ default "..." .metricsUrl }} which provides a sensible default. However, ensure that the .metricsUrl variable is documented in the deployment instructions or README, as this is a new configuration parameter that replaces the old .monitorUrl. Consider adding migration documentation for users upgrading from the old monitor configuration to the new metrics configuration.
| metrics: | |
| metrics: | |
| # metricsUrl is the configurable endpoint for the metrics backend. | |
| # It replaces the legacy monitorUrl parameter; when migrating from older | |
| # configurations that used monitorUrl, set metricsUrl to the same value. | |
| # If metricsUrl is not provided, the following in-cluster VictoriaMetrics | |
| # endpoint is used by default. |
No description provided.