-
Notifications
You must be signed in to change notification settings - Fork 432
RI-7632: rework statistics for api/v2 client #5356
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
RI-7632: rework statistics for api/v2 client #5356
Conversation
Code Coverage - Integration Tests
|
Code Coverage - Backend unit tests
Test suite run success3077 tests passing in 292 suites. Report generated by 🧪jest coverage report action from 33b3db6 |
Code Coverage - Frontend unit tests
Test suite run success5556 tests passing in 704 suites. Report generated by 🧪jest coverage report action from 33b3db6 |
…2-rdi-api-v2-statistics-rework-v2-client
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
...sight/api/src/modules/rdi/client/api/v2/transformers/metrics-collections.transformer.spec.ts
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,90 @@ | |||
| export enum ComponentMetricsCollections { | |||
| processorMetrics = 'processor-metrics', | |||
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.
nit: Very very minor, but just for consistency - in most places we use capitalized enums, so better to stick to it.
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.
had to remove it at all in the second commit. name is inconsistent so we can't rely on it.
| export interface ProcessorMetricsResponse extends ComponentMetricsResponse { | ||
| name: ComponentMetricsCollections.processorMetrics; | ||
| metrics: { | ||
| processing_performance: { |
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.
I didn't compare the interfaces between v1 and v2, I just wonder whether we have matching parts or they're complete independant from each other?
Because, if we have matching sub-parts, maybe we don't have to redeclare them here again?
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.
the root is different when statistics are almost the same but I would like to see complete picture in a single file. and moreover we might have some changes (e.g. additional fields will be added without breaking changes) which might require to modify response interface
d618d15
into
feature/RI-7632-rdi-api-v2-support
What
This PR implements the RDI statistics functionality for the API v2 client, adding support for fetching and transforming statistics data from RDI API v2 endpoints.
Key Changes
Backend (API)
New Statistics Client Methods:
getStatistics()method toApiV2RdiClientfor fetching statistics from RDI API v2Statistics Transformers:
RdiStatisticsTransformerto convert API v2 responses to internal DTOsDTOs and Interfaces:
Testing
ApiV2RdiClient.getStatistics()RdiStatisticsTransformerTechnical Decisions
Transformer Pattern: Used a dedicated transformer class to separate API response parsing from business logic, making the code more maintainable and testable
Type Safety: Defined strict TypeScript interfaces for API v2 responses to catch type mismatches early
Error Handling: Implemented robust error handling to gracefully handle API failures and malformed responses
Separation of Concerns: Kept the client focused on HTTP communication while delegating transformation logic to specialized transformers
Dependencies
This PR builds on top of:
Integration
The statistics client integrates with:
RdiClientProvider- uses this client to fetch statisticsTesting
Automation Tests
ApiV2RdiClient.getStatistics()methodRdiStatisticsTransformerManual Testing
Test Coverage
Note
Medium Risk
Adds a new v2 statistics fetch path and transformation logic, which could affect monitoring/UX if the v2 response shape differs or pipelines aren’t selected as expected. Also introduces typed handling of connection details (including
passwordin the response interface), so reviewers should ensure no sensitive data is surfaced downstream.Overview
Adds API v2 statistics support by introducing
ApiV2RdiClient.getStatistics(), which fetchesapi/v2/pipelines/{name}/metric-collectionsand returns anRdiStatisticsResultwith Success/Fail status handling.Introduces v2 response typing (
GetMetricsCollectionResponse) and a v2 transformer that reuses v1 statistics transformers while extending processing performance withtransform_time_avgandwrite_time_avg, with accompanying unit tests to validate section ordering/types and edge cases.Written by Cursor Bugbot for commit 33b3db6. This will update automatically on new commits. Configure here.