Skip to content

Commit 4ab471e

Browse files
shoummu1crivetimihai
authored andcommitted
fix(observability): fix top performers data loss, dead guards, display bugs, and response time unit conversion (#3794)
* fix metrics top performers panel data loss, dead guards, and display bugs Signed-off-by: Shoumi <shoumimukherjee@gmail.com> * convert avg_response_time seconds to ms in admin.js KPI and metrics cards Signed-off-by: Shoumi <shoumimukherjee@gmail.com> * update vitest assertions and prettier formatting for avg_response_time ms conversion Signed-off-by: Shoumi <shoumimukherjee@gmail.com> * fix: remove dead code from createPerformanceCard, fix CSV zero-value bug, add differential tests - Remove unreachable avgResponseTime/lastExecutionTime branches from createPerformanceCard (its metrics list only has memory/CPU/disk/network keys) - Fix CSV export treating avg_response_time=0 as N/A by using != null instead of truthiness check, consistent with table rendering - Fix trailing whitespace in updateTableRows - Update modularized test assertion for ×1000 conversion (150 → 150000) - Add JS tests for createMetricsCard: seconds→ms conversion, ISO datetime formatting, null handling, zero-value preservation - Add Python test verifying build_top_performers preserves 0.0 values Closes #3793, Closes #3580 Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Shoumi <shoumimukherjee@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
1 parent 0ca6cfd commit 4ab471e

6 files changed

Lines changed: 81 additions & 21 deletions

File tree

mcpgateway/admin_ui/metrics.js

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -774,7 +774,7 @@ export const extractKPIData = function (data) {
774774

775775
const avgResponseTime =
776776
totalExecutions > 0 && weightedResponseSum > 0
777-
? weightedResponseSum / totalExecutions
777+
? (weightedResponseSum / totalExecutions) * 1000
778778
: null;
779779

780780
const successRate =
@@ -1142,12 +1142,12 @@ item.executionCount || item.execution_count || item.executions || 0,
11421142
);
11431143
row.appendChild(execCell);
11441144
1145-
// Avg Response Time
1145+
// Avg Response Time (API returns seconds; convert to ms for display)
11461146
const avgTimeCell = document.createElement("td");
11471147
avgTimeCell.className =
11481148
"px-6 py-4 whitespace-nowrap text-sm text-gray-500 dark:text-gray-300 sm:px-6 sm:py-4";
1149-
const avgTime = item.avg_response_time || item.avgResponseTime;
1150-
avgTimeCell.textContent = avgTime ? `${Math.round(avgTime)}ms` : "N/A";
1149+
const avgTime = item.avg_response_time ?? item.avgResponseTime;
1150+
avgTimeCell.textContent = avgTime != null ? `${Math.round(Number(avgTime) * 1000)}ms` : "N/A";
11511151
row.appendChild(avgTimeCell);
11521152
11531153
// Success Rate
@@ -1489,12 +1489,12 @@ export const updateTableRows = function (
14891489
);
14901490
row.appendChild(execCell);
14911491

1492-
// Avg Response Time
1492+
// Avg Response Time (API returns seconds; convert to ms for display)
14931493
const avgTimeCell = document.createElement("td");
14941494
avgTimeCell.className =
14951495
"px-6 py-4 whitespace-nowrap text-sm text-gray-500 dark:text-gray-300 sm:px-6 sm:py-4";
1496-
const avgTime = item.avg_response_time || item.avgResponseTime;
1497-
avgTimeCell.textContent = avgTime ? `${Math.round(avgTime)}ms` : "N/A";
1496+
const avgTime = item.avg_response_time ?? item.avgResponseTime;
1497+
avgTimeCell.textContent = avgTime != null ? `${Math.round(Number(avgTime) * 1000)}ms` : "N/A";
14981498
row.appendChild(avgTimeCell);
14991499

15001500
// Success Rate
@@ -1554,8 +1554,8 @@ item.execution_count ||
15541554
item.executions ||
15551555
0,
15561556
),
1557-
item.avg_response_time || item.avgResponseTime
1558-
? `${Math.round(item.avg_response_time || item.avgResponseTime)}ms`
1557+
(item.avg_response_time ?? item.avgResponseTime) != null
1558+
? `${Math.round((item.avg_response_time ?? item.avgResponseTime) * 1000)}ms`
15591559
: "N/A",
15601560
`${calculateSuccessRate(item)}%`,
15611561
formatLastUsed(item.last_execution || item.lastExecution),
@@ -1773,7 +1773,20 @@ export const createMetricsCard = function (title, metrics) {
17731773

17741774
const valueSpan = document.createElement("span");
17751775
valueSpan.className = "font-medium dark:text-gray-200";
1776-
valueSpan.textContent = value === "N/A" ? "N/A" : String(value);
1776+
let displayValue;
1777+
if (value === "N/A") {
1778+
displayValue = "N/A";
1779+
} else if (metric.key === "avgResponseTime") {
1780+
const ms = Number(value);
1781+
displayValue = isNaN(ms) ? "N/A" : `${(ms * 1000).toFixed(1)} ms`;
1782+
} else if (metric.key === "lastExecutionTime") {
1783+
displayValue = typeof value === "string" && value.includes("T")
1784+
? value.slice(0, 16).replace("T", " ")
1785+
: String(value);
1786+
} else {
1787+
displayValue = String(value);
1788+
}
1789+
valueSpan.textContent = displayValue;
17771790

17781791
metricRow.appendChild(label);
17791792
metricRow.appendChild(valueSpan);

mcpgateway/templates/metrics_top_performers_partial.html

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,23 +40,27 @@
4040
</td>
4141
<td class="px-4 py-4 whitespace-nowrap text-sm text-gray-900 dark:text-gray-100">
4242
{% set avg_response = item.get('avgResponseTime') %}
43-
{% if avg_response is not none and avg_response != 'N/A' %}
44-
{{ "%.0f"|format(avg_response|float) }}ms
43+
{% if avg_response is not none %}
44+
{{ "%.0f"|format((avg_response|float) * 1000) }}ms
4545
{% else %}
4646
N/A
4747
{% endif %}
4848
</td>
4949
<td class="px-4 py-4 whitespace-nowrap text-sm">
50-
{% set success_rate = item.get('successRate', 0) or 0 %}
50+
{% set success_rate = item.get('successRate') %}
51+
{% if success_rate is not none %}
5152
<span class="inline-flex items-center px-2.5 py-0.5 rounded-full text-xs font-semibold
5253
{% if success_rate >= 95 %}bg-green-100 text-green-800{% elif success_rate >= 80 %}bg-yellow-100 text-yellow-800{% else %}bg-red-100 text-red-800{% endif %}">
5354
{{ "%.1f"|format(success_rate) }}%
5455
</span>
56+
{% else %}
57+
N/A
58+
{% endif %}
5559
</td>
5660
<td class="px-4 py-4 whitespace-nowrap text-sm text-gray-900 dark:text-gray-100">
5761
{% set last_exec = item.get('lastExecution') %}
58-
{% if last_exec and last_exec != 'Never' %}
59-
{{ last_exec }}
62+
{% if last_exec %}
63+
{{ last_exec[:16] | replace('T', ' ') }}
6064
{% else %}
6165
Never
6266
{% endif %}

mcpgateway/utils/metrics_common.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,8 @@ def build_top_performers(results: List) -> List[TopPerformer]:
5757
id=result.id,
5858
name=result.name,
5959
execution_count=result.execution_count or 0,
60-
avg_response_time=float(result.avg_response_time) if result.avg_response_time else None,
61-
success_rate=float(result.success_rate) if result.success_rate else None,
60+
avg_response_time=float(result.avg_response_time) if result.avg_response_time is not None else None,
61+
success_rate=float(result.success_rate) if result.success_rate is not None else None,
6262
last_execution=result.last_execution,
6363
)
6464
for result in results

tests/js/admin-parsing.test.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -272,7 +272,7 @@ describe("extractKPIData", () => {
272272
expect(result.totalExecutions).toBe(100);
273273
expect(result.successRate).toBe(90);
274274
expect(result.errorRate).toBe(10);
275-
expect(result.avgResponseTime).toBeCloseTo(1.5, 1);
275+
expect(result.avgResponseTime).toBeCloseTo(1500, 1);
276276
});
277277

278278
test("aggregates across multiple categories", () => {
@@ -362,8 +362,8 @@ describe("extractKPIData", () => {
362362
},
363363
};
364364
const result = extractKPIData(data);
365-
// Weighted avg = (100*2.0 + 100*4.0) / 200 = 3.0
366-
expect(result.avgResponseTime).toBeCloseTo(3.0, 1);
365+
// Weighted avg = (100*2.0 + 100*4.0) / 200 = 3.0s = 3000ms
366+
expect(result.avgResponseTime).toBeCloseTo(3000, 1);
367367
});
368368

369369
test("ignores N/A response time values", () => {

tests/unit/js/metrics.test.js

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -609,7 +609,7 @@ describe("extractKPIData", () => {
609609
};
610610

611611
const kpi = extractKPIData(data);
612-
expect(kpi.avgResponseTime).toBe(150); // Weighted average
612+
expect(kpi.avgResponseTime).toBe(150000); // Weighted average (seconds × 1000 → ms)
613613
});
614614

615615
test("handles null avgResponseTime", () => {
@@ -825,6 +825,7 @@ describe("createPerformanceCard", () => {
825825
expect(card).toBeInstanceOf(HTMLElement);
826826
consoleError.mockRestore();
827827
});
828+
828829
});
829830

830831
describe("createRecentActivitySection", () => {
@@ -904,6 +905,31 @@ describe("createMetricsCard", () => {
904905
const card = createMetricsCard("Prompts", {});
905906
expect(card.textContent).toContain("N/A");
906907
});
908+
909+
test("converts avgResponseTime from seconds to milliseconds", () => {
910+
const metrics = { avgResponseTime: 1.5 };
911+
const card = createMetricsCard("Tools", metrics);
912+
expect(card.textContent).toContain("1500.0 ms");
913+
});
914+
915+
test("formats lastExecutionTime as human-readable date", () => {
916+
const metrics = { lastExecutionTime: "2026-03-13T06:00:00.000000" };
917+
const card = createMetricsCard("Tools", metrics);
918+
expect(card.textContent).toContain("2026-03-13 06:00");
919+
});
920+
921+
test("displays N/A for null avgResponseTime", () => {
922+
const metrics = { avgResponseTime: null };
923+
const card = createMetricsCard("Tools", metrics);
924+
// avgResponseTime null → value resolves to "N/A" via ?? chain
925+
expect(card.textContent).toContain("N/A");
926+
});
927+
928+
test("preserves zero avgResponseTime", () => {
929+
const metrics = { avgResponseTime: 0 };
930+
const card = createMetricsCard("Tools", metrics);
931+
expect(card.textContent).toContain("0.0 ms");
932+
});
907933
});
908934

909935
// ===================================================================

tests/unit/mcpgateway/utils/test_metrics_common.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,3 +44,20 @@ def test_build_top_performers_handles_missing_values():
4444
assert performers[0].avg_response_time is None
4545
assert performers[0].success_rate is None
4646
assert performers[0].last_execution is None
47+
48+
49+
def test_build_top_performers_preserves_zero_values():
50+
"""Zero avg_response_time and success_rate must not be coerced to None."""
51+
result = SimpleNamespace(
52+
id=3,
53+
name="zero-tool",
54+
execution_count=10,
55+
avg_response_time=0.0,
56+
success_rate=0.0,
57+
last_execution=None,
58+
)
59+
60+
performers = build_top_performers([result])
61+
62+
assert performers[0].avg_response_time == 0.0
63+
assert performers[0].success_rate == 0.0

0 commit comments

Comments
 (0)