feat(frontend): metrics-sdk support and devbox implement#6613
feat(frontend): metrics-sdk support and devbox implement#6613zjy365 merged 18 commits intolabring:mainfrom
Conversation
Whoa! Easy there, Partner!This PR is too big. Please break it up into smaller PRs. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new TypeScript SDK package (sealos-metrics-sdk) for querying Prometheus/Victoria Metrics directly from Next.js applications. The SDK provides built-in Kubernetes authentication and type-safe interfaces for querying metrics across launchpad applications, databases, and MinIO storage.
Changes:
- New metrics-sdk package with TypeScript client for Prometheus/Victoria Metrics queries
- Authentication service using @kubernetes/client-node for K8s permission validation
- Service modules for launchpad, database, MinIO, and raw metric queries with PromQL builders
- Rollup build configuration for dual-format output (ESM and CJS)
- Comprehensive documentation and test utilities
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/pnpm-lock.yaml | Added dependencies for new metrics-sdk package including axios, dayjs, rollup plugins, and esbuild 0.27.2 |
| frontend/packages/metrics-sdk/package.json | Package configuration with dual exports (ESM/CJS), peer dependency on @kubernetes/client-node |
| frontend/packages/metrics-sdk/tsconfig.json | TypeScript configuration with ES2017 target and strict mode enabled |
| frontend/packages/metrics-sdk/rollup.config.js | Build configuration for bundling TypeScript with dual output formats |
| frontend/packages/metrics-sdk/src/client.ts | Main SDK client exposing launchpad, database, minio, and raw query services |
| frontend/packages/metrics-sdk/src/auth/auth.ts | Kubernetes authentication service with kubeconfig parsing and permission validation |
| frontend/packages/metrics-sdk/src/services/*.ts | Service implementations for different metric types with PromQL query builders |
| frontend/packages/metrics-sdk/src/types/*.ts | TypeScript type definitions for query parameters and responses |
| frontend/packages/metrics-sdk/src/constants/promql.ts | PromQL query templates for various metric types and databases |
| frontend/packages/metrics-sdk/src/utils/time.ts | Utility functions for timestamp formatting and conversion |
| frontend/packages/metrics-sdk/test/index.ts | Manual test script for SDK validation |
| frontend/packages/metrics-sdk/README.md | Comprehensive documentation with usage examples and API reference |
Files not reviewed (1)
- frontend/pnpm-lock.yaml: Language not supported
| console.log('🔍 Debug Info:'); | ||
| console.log(' Query:', query); | ||
| console.log(' Endpoint:', this.baseURL + endpoint); | ||
| console.log(' Params:', formData); | ||
|
|
||
| const body = new URLSearchParams( | ||
| Object.entries(formData).reduce<Record<string, string>>((acc, [key, value]) => { | ||
| acc[key] = String(value); | ||
| return acc; | ||
| }, {}) | ||
| ).toString(); | ||
| const response = await this.client.post(endpoint, body); | ||
|
|
||
| console.log(' Response status:', response.data.status); | ||
| console.log(' Result count:', response.data.data?.result?.length || 0); | ||
| if (response.data.data?.result?.length === 0) { | ||
| console.log(' ⚠️ Empty result!'); | ||
| } | ||
| console.log(''); |
There was a problem hiding this comment.
Debug logging statements should be removed before production. These console.log calls will execute on every query and expose sensitive information like query parameters and results. Consider using a configurable logger or removing these debug statements entirely.
| ``` | ||
|
|
||
| **Replaces:** | ||
|
|
There was a problem hiding this comment.
The README states the SDK "Replaces" three Go services (launchpad-monitor, database-monitor, minio-monitor), but this is misleading in a PR description. This new SDK provides an alternative client-side implementation but does not actually remove those services. Consider rephrasing to "Provides alternative to" or "Can replace" to avoid confusion.
| ``` | |
| **Replaces:** | |
| ``` | |
| **Can replace / provides alternative to:** |
| console.error('❌ KUBECONFIG environment variable not set'); | ||
| process.exit(1); |
There was a problem hiding this comment.
Error handling issue: When kubeconfig is empty, the code continues execution and will fail later with less clear error messages. It would be better to throw an error immediately with a descriptive message rather than exiting the process, allowing the calling code to handle the error appropriately.
| console.error('❌ KUBECONFIG environment variable not set'); | |
| process.exit(1); | |
| throw new Error('❌ KUBECONFIG environment variable not set'); |
|
|
||
| export interface MinioQueryParams extends BaseQueryParams { | ||
| query: MinioMetric; | ||
| type: 'minio'; |
There was a problem hiding this comment.
API design inconsistency: The MinioQueryParams interface requires both 'query' (typed as MinioMetric enum) and 'type' (hardcoded as literal 'minio'). The 'type' field seems redundant since the service is already minio-specific. This differs from other services where 'type' has semantic meaning. Consider removing the 'type' field or making it optional.
| type: 'minio'; | |
| type?: 'minio'; |
| constructor(kubeconfig: string) { | ||
| this.kubeconfig = kubeconfig; |
There was a problem hiding this comment.
Security: The kubeconfig string is stored in memory and passed around. If this contains sensitive tokens or certificates, it could be exposed through error messages, logs, or memory dumps. Consider using more secure credential handling patterns, such as extracting only the necessary auth tokens and discarding the full kubeconfig after initial parsing.
|
|
||
| export const toSeconds = (value: number | string): number => { | ||
| const num = typeof value === 'string' ? Number(value) : value; | ||
| return Math.abs(num) > 1e12 ? Math.floor(num / 1000) : Math.floor(num); |
There was a problem hiding this comment.
Inconsistent timestamp handling: The toSeconds function checks if the absolute value is greater than 1e12 to determine if it's in milliseconds, but this approach could fail for timestamps far in the future or past. A more reliable approach would be to check the actual magnitude or require explicit units. Additionally, using Math.abs could cause issues with negative timestamps (before epoch).
| return Math.abs(num) > 1e12 ? Math.floor(num / 1000) : Math.floor(num); | |
| if (!Number.isFinite(num)) { | |
| return NaN; | |
| } | |
| // Infer unit based on proximity to current time in seconds vs milliseconds. | |
| // This avoids misclassifying far-future timestamps purely by magnitude. | |
| const nowMs = Date.now(); | |
| const nowSec = Math.floor(nowMs / 1000); | |
| const tenYearsInSeconds = 10 * 365 * 24 * 60 * 60; | |
| const tenYearsInMs = tenYearsInSeconds * 1000; | |
| // If it's close to the current seconds-based timestamp, treat as seconds. | |
| if (Math.abs(num - nowSec) <= tenYearsInSeconds) { | |
| return Math.floor(num); | |
| } | |
| // If it's close to the current milliseconds-based timestamp, treat as milliseconds. | |
| if (Math.abs(num - nowMs) <= tenYearsInMs) { | |
| return Math.floor(num / 1000); | |
| } | |
| // Fallback: treat as seconds to avoid misclassifying extreme values as milliseconds. | |
| return Math.floor(num); |
| export class MinioService extends BaseMetricsService { | ||
| private minioInstance: string; | ||
|
|
||
| constructor(baseURL: string, authService: any, minioInstance?: string) { |
There was a problem hiding this comment.
The 'any' type annotation reduces type safety. Consider using a more specific type for the authService parameter. Based on the BaseMetricsService constructor, this should be typed as 'AuthService' instead of 'any'.
| constructor(baseURL: string, authService: any, minioInstance?: string) { | |
| constructor( | |
| baseURL: string, | |
| authService: ConstructorParameters<typeof BaseMetricsService>[1], | |
| minioInstance?: string | |
| ) { |
|
|
||
| constructor(baseURL: string, authService: any, minioInstance?: string) { | ||
| super(baseURL, authService); | ||
| this.minioInstance = minioInstance || process.env.OBJECT_STORAGE_INSTANCE || ''; |
There was a problem hiding this comment.
Missing error handling for environment variable access. If OBJECT_STORAGE_INSTANCE is not set and minioInstance is not provided, this will default to an empty string which could lead to incorrect query construction. Consider throwing an error or documenting this behavior clearly.
| this.minioInstance = minioInstance || process.env.OBJECT_STORAGE_INSTANCE || ''; | |
| const resolvedInstance = minioInstance ?? process.env.OBJECT_STORAGE_INSTANCE; | |
| if (!resolvedInstance) { | |
| throw new Error( | |
| 'MinIO instance is not configured. Provide a minioInstance argument or set the OBJECT_STORAGE_INSTANCE environment variable.' | |
| ); | |
| } | |
| this.minioInstance = resolvedInstance; |
| import { resolve, dirname } from 'path'; | ||
| import { fileURLToPath } from 'url'; | ||
|
|
||
| const __dirname = dirname(fileURLToPath(import.meta.url)); |
There was a problem hiding this comment.
Security concern: The test file contains hardcoded credential expectations via environment variables. While using environment variables is better than hardcoding values, the test file should include clear warnings about not committing .env files with actual credentials. Consider adding a .env.example file and documenting this in comments.
| const __dirname = dirname(fileURLToPath(import.meta.url)); | |
| const __dirname = dirname(fileURLToPath(import.meta.url)); | |
| // NOTE: This test expects configuration via environment variables loaded from a local `.env` file. | |
| // - Do NOT commit any `.env` files containing real credentials or kubeconfig information. | |
| // - Instead, add a `.env.example` file (with non-sensitive placeholder values) to version control | |
| // and copy it to `.env` locally when running this test. | |
| // - Ensure `.env` is listed in `.gitignore` (or equivalent ignore configuration). |
| export const LAUNCHPAD_QUERIES = { | ||
| cpu: 'round(sum(node_namespace_pod_container:container_cpu_usage_seconds_total:sum_irate{namespace=~"$namespace",pod=~"$pod.*"}) by (pod) / sum(cluster:namespace:pod_cpu:active:kube_pod_container_resource_limits{namespace=~"$namespace",pod=~"$pod.*"}) by (pod) * 100,0.01)', | ||
| memory: | ||
| 'round(sum(container_memory_working_set_bytes{job="kubelet", metrics_path="/metrics/cadvisor",namespace=~"$namespace",pod=~"$pod.*"}) by(pod) / sum(cluster:namespace:pod_memory:active:kube_pod_container_resource_limits{namespace=~"$namespace",pod=~"$pod.*"}) by (pod)* 100, 0.01)', | ||
| average_cpu: | ||
| 'avg(round(sum(node_namespace_pod_container:container_cpu_usage_seconds_total:sum_irate{namespace=~"$namespace",pod=~"$pod.*"}) by (pod) / sum(cluster:namespace:pod_cpu:active:kube_pod_container_resource_limits{namespace=~"$namespace",pod=~"$pod.*"}) by (pod) * 100,0.01))', | ||
| average_memory: | ||
| 'avg(round(sum(container_memory_working_set_bytes{job="kubelet", metrics_path="/metrics/cadvisor",namespace=~"$namespace",pod=~"$pod.*",container!=""}) by(pod) / sum(cluster:namespace:pod_memory:active:kube_pod_container_resource_limits{namespace=~"$namespace",pod=~"$pod.*"}) by (pod) * 100, 0.01))', | ||
| storage: | ||
| 'round((max by (persistentvolumeclaim,namespace) (kubelet_volume_stats_used_bytes {namespace=~"$namespace", persistentvolumeclaim=~"@persistentvolumeclaim"})) / (max by (persistentvolumeclaim,namespace) (kubelet_volume_stats_capacity_bytes {namespace=~"$namespace", persistentvolumeclaim=~"@persistentvolumeclaim"})) * 100, 0.01)' | ||
| } as const; |
There was a problem hiding this comment.
Type safety issue: The type assertion 'as const' on line 11 creates a readonly tuple type, but the actual LAUNCHPAD_QUERIES object uses string keys like 'cpu', 'memory', etc. This mismatch means the LaunchpadMetric enum values must exactly match these keys. Consider adding a type constraint to ensure the enum and query keys stay in sync, or document this requirement clearly.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6613 +/- ##
=======================================
Coverage 61.86% 61.86%
=======================================
Files 8 8
Lines 653 653
=======================================
Hits 404 404
Misses 202 202
Partials 47 47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* feat: basic feat * feat: test * feat: perf code to full feat * chore: update readme.md * feat: optional namespace * perf: podName logic adjust * chore: Prometheus->Metrics * chore: test adjust * chore: test logResult * chore: readme.zh-CN.md * chore: remove minio type * chore: adjust launchpad storage to disk * chore: transform enum to type * fix: try to fix ts bug * feat: devbox metrics support * feat: adjust other route * fix: whitelistKubernetesHosts sdk transform bug * feat: cache
…ring#6613)" This reverts commit d0ba22f.
No description provided.