Conversation
…t scraped metrics Resolves kafbat#1958
|
AI Summary The metrics endpoint incorrectly returns a 404 for valid brokers without configured metrics or collected data, causing frontend crashes. This PR resolves the issue by returning a 200 status code with an empty metrics list for known brokers, while reserving 404 responses strictly for unknown broker IDs. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough
ChangesBroker metrics retrieval
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change makes known brokers return an empty metrics list instead of 404 while preserving 404 responses for unknown brokers; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Hi alilxxey! 👋
Welcome, and thank you for opening your first PR in the repo!
Please wait for triaging by our maintainers.
Please take a look at our contributing guide.
What changes did you make? (Give an overview)
Resolves #1958 and #1942
GET /api/clusters/{clusterName}/brokers/{id}/metricsreturned 404 for every broker when the cluster has nometricssection configured (and for brokers whose scrape yielded nothing), which crashes the frontend Metrics tab into a blank page (useSuspenseQuerythrows, the Brokers route has no error boundary). This is a regression from the metrics rework #1208: up to v1.3.0MetricsCollectorkept an entry per live node (empty list when nothing was collected), so the endpoint returned200 {"metrics":[]}.This PR restores those semantics at the service layer:
200 {"metrics":[]}), covering both the metrics-not-configured case and per-broker scrape gaps (related: Metrics tab of MSK broker results in a 404 error instead of metrics gathered from open monitoring endpoint #1630);NotFoundException(mirrors the existing check inBrokerService#getBrokersConfig), which the controller maps to 404.No frontend changes needed: with a valid
200response the Metrics tab renders the JSON as before.Is there anything you'd like reviewers to focus on?
The chosen semantics: 404 is now reserved for unknown broker ids; a known broker always gets
200with whatever has been scraped (possibly empty). An alternative was to populate per-node empty entries inMetricsScraper#scrapeBrokers, but the service-layer guard also covers scrape gaps for configured clusters.How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)
Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)
Check out Contributing and Code of Conduct
Summary by CodeRabbit
Bug Fixes
Tests