-
Notifications
You must be signed in to change notification settings - Fork 128
backport (4.x): Switch to Observability Pipeline endpoints for querying metrics #1578
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
Conversation
* Switch to new endpoint for querying metrics * Address lint reports
This adjusts the backport to work with the 4.x codebase: - Convert all metrics commands to use CommandBase methods instead of the Selector class - Remove constructor dependencies and use service locator pattern via getService() - Use filterEnvsMaybeActive(), validateInput(), and getSelectedEnvironment() for environment selection - Remove CurlCommand reference from Application.php (command was deleted in backport) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Replace dependency injection with service locator pattern:
- Change $this->io->method() to $this->method() calls
- Change $this->api->method() to $this->api()->method() calls
- Use $this->getService('property_formatter') with local variable
- Fix Format::format() static method call (was trying to call instance method on string)
- Fix SourceField property access (remove ->value as they're now strings, not enums)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
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 backports changes from #1536 to switch from the legacy metrics API to the new Observability Pipeline endpoints. The refactoring significantly simplifies the metrics querying architecture while introducing some minor breaking changes in the API behavior.
Key Changes:
- Replaced POST-based metrics queries with GET requests to the Observability Pipeline
/resources/overviewendpoint - Removed the deprecated
--intervaloption and themetrics:curlcommand - Refactored metrics data structures to use
SourceFieldandSourceFieldPercentageclasses instead ofSketch - Changed default behavior to expose
routermetrics and displayinodescolumns by default
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Model/Metrics/TimeSpec.php | Removed interval property and related methods as the new API handles intervals internally |
| src/Model/Metrics/Query.php | Refactored to support GET-based queries with services, types, and aggregations parameters |
| src/Model/Metrics/Field.php | Simplified from formatting logic to a data container with format, value, and warn properties |
| src/Model/Metrics/SourceField.php | New class representing a metric source with aggregation and optional mountpoint |
| src/Model/Metrics/SourceFieldPercentage.php | New class for calculating percentage metrics from two source fields |
| src/Model/Metrics/MetricKind.php | New constants class defining metric types and API parameters |
| src/Model/Metrics/Format.php | Extracted formatting logic from Field class into standalone utility |
| src/Model/Metrics/Aggregation.php | New constants class for aggregation types |
| src/Model/Metrics/Sketch.php | Removed legacy Sketch class no longer needed with new API |
| src/Command/Metrics/MetricsCommandBase.php | Major refactoring to support new API with processQuery() and simplified data extraction |
| src/Command/Metrics/*Command.php | Updated all metric commands to use new data structures and API |
| src/Command/Metrics/CurlCommand.php | Removed deprecated command |
| src/Application.php | Removed CurlCommand registration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
akalipetis
left a comment
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.
LGTM
Backports #1536 - as that MR says: