-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix(monitoring): resolve container name collision and chart Y-axis issues #3279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: canary
Are you sure you want to change the base?
Conversation
…sues - Use exact match for container name queries instead of LIKE prefix matching to prevent services with similar names (e.g., 'myapp' and 'myapp-test') from having their monitoring data mixed together - Make CPU chart Y-axis domain dynamic to handle values >100% which can occur on multi-core systems where Docker reports aggregate CPU usage Fixes Dokploy#3182
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR attempts to fix two issues: container name collision in monitoring queries and CPU chart Y-axis display problems. However, the SQL query changes introduce a critical bug that will break all metric retrieval.
Key changes:
- Replaced SQL LIKE prefix matching with exact equality for container name queries
- Made CPU chart Y-axis domains dynamic based on actual data values
- Removed string manipulation logic from database query functions
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/monitoring/database/containers.go | Changed SQL queries from LIKE to exact match and removed container name suffix-stripping logic - introduces critical bug |
| apps/dokploy/components/dashboard/monitoring/paid/servers/cpu-chart.tsx | Added dynamic Y-axis domain calculation with proper null handling |
| apps/dokploy/components/dashboard/monitoring/paid/container/container-cpu-chart.tsx | Added dynamic Y-axis domain calculation but missing null value safeguards |
| apps/dokploy/components/dashboard/monitoring/free/container/docker-cpu-chart.tsx | Added dynamic Y-axis domain calculation with proper null handling |
Comments suppressed due to low confidence (2)
apps/monitoring/database/containers.go:59
- This change from LIKE prefix matching to exact matching will break metric retrieval. The database stores the full container name (e.g., "myapp-1") via SaveContainerMetric which saves container.Name. However, the appName parameter passed to this function from the API is the service name without the numeric suffix (e.g., "myapp").
In the previous implementation, GetServiceName logic stripped the suffix, allowing "myapp" to match "myapp-1" via LIKE. With exact matching, queries for "myapp" will return no results when the database contains "myapp-1".
To fix the container name collision issue while preserving functionality, consider storing a normalized service name separately in the database or applying the GetServiceName transformation when saving metrics.
func (db *DB) GetLastNContainerMetrics(containerName string, limit int) ([]ContainerMetric, error) {
query := `
WITH recent_metrics AS (
SELECT metrics_json
FROM container_metrics
WHERE container_name = ?
ORDER BY timestamp DESC
LIMIT ?
)
SELECT metrics_json FROM recent_metrics ORDER BY json_extract(metrics_json, '$.timestamp') ASC
`
apps/monitoring/database/containers.go:92
- This change from LIKE prefix matching to exact matching will break metric retrieval for the same reason as GetLastNContainerMetrics. The database stores full container names with suffixes (e.g., "myapp-1") but queries use service names without suffixes (e.g., "myapp"), resulting in no matches with exact equality comparison.
func (db *DB) GetAllMetricsContainer(containerName string) ([]ContainerMetric, error) {
query := `
WITH recent_metrics AS (
SELECT metrics_json
FROM container_metrics
WHERE container_name = ?
ORDER BY timestamp DESC
)
SELECT metrics_json FROM recent_metrics ORDER BY json_extract(metrics_json, '$.timestamp') ASC
`
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| timestamp: "", | ||
| cpu: 0, | ||
| }; | ||
| const maxCPU = Math.max(...formattedData.map((d) => d.cpu), 100); |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If formattedData contains items where cpu is undefined or null, Math.max will receive these values and may return NaN, causing the Y-axis domain to be invalid. Consider adding a fallback value in the map function to handle missing cpu values, similar to how cpu-chart.tsx uses d.cpu || 0.
| const maxCPU = Math.max(...formattedData.map((d) => d.cpu), 100); | |
| const maxCPU = Math.max(...formattedData.map((d) => d.cpu || 0), 100); |
… null guard - Pass serviceName to processContainerMetrics() and use it for metric Name field instead of container.Name, ensuring exact match queries work correctly - Add || 0 fallback in container-cpu-chart.tsx to prevent NaN in Y-axis domain calculation when cpu values are null/undefined Addresses PR review feedback on incomplete SQL query fix and missing null handling.
Addressed Review FeedbackFixed both issues identified in the review: 1. SQL Query Fix (Critical)The original change from Fix: Updated Changes in
2. Null Handling FixAdded the missing Change in - const maxCPU = Math.max(...formattedData.map((d) => d.cpu), 100);
+ const maxCPU = Math.max(...formattedData.map((d) => d.cpu || 0), 100); |
Summary
Container name collision fix: Changed SQL queries to use exact matching (
=) instead of fuzzy LIKE prefix matching (LIKE ? || '%'). This prevents services with similar names (e.g.,myappandmyapp-test) from having their monitoring data incorrectly mixed together.CPU chart Y-axis fix: Made the Y-axis domain dynamic based on actual data values instead of using a fixed
[0, 100]range. This allows proper display of CPU usage values >100% which can occur on multi-core systems where Docker reports aggregate CPU usage.Test plan
myappandmyapp-test)Fixes #3182