NETOBSERV-1828: Refactor frontend and backend for loki/prometheus datasource#1581
Conversation
NETOBSERV-1828 The file web/src/api/loki.ts contained generic response types (AggregatedQueryResponse, Stats, TopologyMetrics, NetflowMetrics, etc.) shared by both Loki and Prometheus query paths. Having it named after Loki was misleading since these types have nothing Loki-specific about them. Rename to query-response.ts and update all ~40 import sites across components, utils, models, and test files. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Skipping CI for Draft Pull Request. |
|
@memodi: This pull request references NETOBSERV-1828 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Warning Review limit reached
More reviews will be available in 47 minutes and 49 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughMoves shared query response types ( ChangesQuery response model, merger, and topology input extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 2 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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 |
|
@jpinsonneau - I took JIRA description to Claude and it mainly changed |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1581 +/- ##
==========================================
- Coverage 52.43% 52.22% -0.22%
==========================================
Files 236 239 +3
Lines 12897 12963 +66
Branches 1659 1666 +7
==========================================
+ Hits 6763 6770 +7
- Misses 5493 5551 +58
- Partials 641 642 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Yes it's a deeper refactor and not only on frontend. Backend was made based on loki and we added prometheus after. For example There is also some duplication between files for configuring clients, setting query filters etc. We may want to improve that in that PR. |
…ages - Move generic QueryResponse/AggregatedQueryResponse types from pkg/model/loki.go to pkg/model/query_response.go (Loki-only types remain) - Extract Merger interface and MatrixMerger to pkg/merger package - Extract TopologyInput struct and GetLabelsAndFilter to pkg/utils/queryparams - Update pkg/loki to use type aliases for backward compatibility - Update pkg/prometheus/query.go to import queryparams instead of loki - Move matrix_merger_test.go to pkg/merger (unexported field access) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r pkg directly - Update pkg/handler/clients.go to import merger.Merger instead of loki.Merger - Update pkg/handler/topology.go to use merger.NewMatrixMerger directly - Delete pkg/loki/merger.go and pkg/loki/matrix_merger.go (aliases no longer needed) - Remove vestigial Merger interface embed from StreamMerger struct Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
/ok-to-test |
openshift-ci
Bot
added
the
ok-to-test
|
New images: quay.io/netobserv/network-observability-console-plugin:3c53ad40
quay.io/netobserv/network-observability-standalone-frontend:3c53ad40They will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=3c53ad40 make set-plugin-image |
memodi
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/merger/matrix_merger.go (1)
12-21: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove vestigial Merger interface embed.
Line 14 embeds the
Mergerinterface inMatrixMerger, but the struct already implementsMergerthrough itsAdd()andGet()methods (lines 40, 81). Embedding an interface that the struct implements doesn't add functionality and creates confusion about the type's contract.♻️ Proposed fix
// MatrixMerger aggregates multiple Matrix results from Loki or Prometheus queries. type MatrixMerger struct { - Merger index map[string]indexedSampleStream merged model.Matrix stats []interface{}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/merger/matrix_merger.go` around lines 12 - 21, The MatrixMerger struct embeds the Merger interface as an unnamed field, but this is redundant since MatrixMerger already implements the Merger interface through its Add() and Get() methods. Remove the Merger interface embedding from the MatrixMerger struct definition by deleting the standalone Merger line, keeping only the named fields like index, merged, stats, numQueries, reqLimit, and limitReached.pkg/model/loki.go (1)
1-1: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUpdate package comment to reflect actual contents.
The package comment mentions "shared query response types" but those have been moved to
pkg/model/query_response.go. This file now contains only Loki-specific types (Streams, Stream, Entry, LabelValuesResponse).📝 Suggested fix
-// Package model contains data models for Loki-specific types and shared query response types. +// Package model contains data models for Loki-specific types (streams, entries, and label responses).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/model/loki.go` at line 1, The package comment at the beginning of loki.go incorrectly states that the package contains "shared query response types" when those have been moved to pkg/model/query_response.go. Update the package comment to accurately reflect that this file contains only Loki-specific types (Streams, Stream, Entry, LabelValuesResponse), removing the reference to shared query response types that no longer reside in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/merger/matrix_merger.go`:
- Around line 12-21: The MatrixMerger struct embeds the Merger interface as an
unnamed field, but this is redundant since MatrixMerger already implements the
Merger interface through its Add() and Get() methods. Remove the Merger
interface embedding from the MatrixMerger struct definition by deleting the
standalone Merger line, keeping only the named fields like index, merged, stats,
numQueries, reqLimit, and limitReached.
In `@pkg/model/loki.go`:
- Line 1: The package comment at the beginning of loki.go incorrectly states
that the package contains "shared query response types" when those have been
moved to pkg/model/query_response.go. Update the package comment to accurately
reflect that this file contains only Loki-specific types (Streams, Stream,
Entry, LabelValuesResponse), removing the reference to shared query response
types that no longer reside in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 320e41c3-5437-4da8-b1fc-baf24bd89a7b
📒 Files selected for processing (65)
pkg/handler/clients.gopkg/handler/topology.gopkg/loki/merger.gopkg/loki/streams_merger.gopkg/loki/topology_query.gopkg/merger/matrix_merger.gopkg/merger/matrix_merger_test.gopkg/merger/merger.gopkg/model/loki.gopkg/model/query_response.gopkg/prometheus/query.gopkg/prometheus/query_test.gopkg/utils/queryparams/topology.goweb/src/api/query-response.tsweb/src/api/routes.tsweb/src/components/__tests-data__/flows.tsweb/src/components/__tests-data__/metrics.tsweb/src/components/__tests__/netflow-tab.spec.tsxweb/src/components/__tests__/netflow-traffic.spec.tsxweb/src/components/drawer/element/__tests__/element-panel.spec.tsxweb/src/components/drawer/element/element-field.tsxweb/src/components/drawer/element/element-panel-metrics.tsxweb/src/components/drawer/element/element-panel-stats.tsxweb/src/components/drawer/element/element-panel.tsxweb/src/components/drawer/netflow-traffic-drawer.tsxweb/src/components/messages/empty.tsxweb/src/components/messages/error.tsxweb/src/components/messages/status-texts.tsxweb/src/components/metrics/__tests__/metrics-donut.spec.tsxweb/src/components/metrics/histogram.tsxweb/src/components/metrics/metrics-donut.tsxweb/src/components/metrics/metrics-graph-total.tsxweb/src/components/metrics/metrics-graph.tsxweb/src/components/netflow-traffic.tsxweb/src/components/query-summary/__tests__/metrics-query-summary.spec.tsxweb/src/components/query-summary/__tests__/summary-panel.spec.tsxweb/src/components/query-summary/flows-query-summary.tsxweb/src/components/query-summary/metrics-query-summary-content.tsxweb/src/components/query-summary/metrics-query-summary.tsxweb/src/components/query-summary/summary-panel-content.tsxweb/src/components/query-summary/summary-panel.tsxweb/src/components/tabs/netflow-overview/__tests__/netflow-overview.spec.tsxweb/src/components/tabs/netflow-overview/netflow-overview.tsxweb/src/components/tabs/netflow-table/netflow-table.tsxweb/src/components/tabs/netflow-topology/2d/styles/styleNode.tsxweb/src/components/tabs/netflow-topology/2d/topology-content.tsxweb/src/components/tabs/netflow-topology/__tests-data__/metrics.tsweb/src/components/tabs/netflow-topology/__tests__/netflow-topology.spec.tsxweb/src/components/tabs/netflow-topology/netflow-topology.tsxweb/src/components/tabs/netflow-topology/peer-resource-link.tsxweb/src/components/toolbar/filters/__tests__/filters-toolbar.spec.tsxweb/src/components/toolbar/filters/summary-filter-button.tsxweb/src/components/toolbar/histogram-toolbar.tsxweb/src/model/metrics.tsweb/src/model/netflow-context.tsweb/src/model/topology.tsweb/src/utils/__tests__/back-and-forth.spec.tsweb/src/utils/__tests__/metrics.spec.tsweb/src/utils/__tests__/netflow-fetching-hook.spec.tsweb/src/utils/back-and-forth.tsweb/src/utils/filters-helper.tsweb/src/utils/ids.tsweb/src/utils/metrics-helper.tsweb/src/utils/metrics.tsweb/src/utils/netflow-fetching-hook.ts
💤 Files with no reviewable changes (3)
- web/src/utils/filters-helper.ts
- pkg/loki/merger.go
- pkg/loki/streams_merger.go
jpinsonneau
left a comment
jpinsonneau
left a comment
There was a problem hiding this comment.
LGTM, just a small comment.
Also, the comment on matrixMerger from the rabbit seems legit 😸
| } | ||
| // TopologyInput is an alias kept for backwards compatibility within this package. | ||
| // New code should use queryparams.TopologyInput directly. | ||
| type TopologyInput = queryparams.TopologyInput |
There was a problem hiding this comment.
can't we point the queryparams.TopologyInput directly and remove that ?
…irectly - Remove TopologyInput type alias from pkg/loki/topology_query.go - Update pkg/handler/topology.go and pkg/loki/topology_query.go to use queryparams.TopologyInput directly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions
Bot
removed
the
ok-to-test
|
@memodi: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@jpinsonneau - any more comments here ? |
jpinsonneau
left a comment
jpinsonneau
left a comment
There was a problem hiding this comment.
We should be good to go ! Thanks @memodi !
|
/cherry-pick main-pf5 |
|
@memodi: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: memodi The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit 8c99f38
into
netobserv:main
|
@memodi: new pull request created: #1588 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
4 participants
What
Refactor and organizes frontend and backend for loki/prometheus datasource.
Backend
pkg/model/loki.go: move generic query response types (QueryResponse, AggregatedQueryResponse, ResultType, Matrix, Vector, Scalar) to new pkg/model/query_response.go; keep only Loki-specific types (Stream, Entry, LabelValuesResponse, flow line mappings) in loki.gopkg/mergerpackage — both were in pkg/loki but MatrixMerger is used for Prometheus matrix results toopkg/handler/clients.goto use merger.Merger directly instead of loki.Mergerpkg/handler/topology.goto use merger.NewMatrixMerger directly instead of loki.NewMatrixMergerpkg/prometheus/query.goand its tests to import queryparams directly, removing the dependency on pkg/loki🤖 Generated with Claude Code via
/jira:solve https://redhat.atlassian.net/browse/NETOBSERV-1828Summary by CodeRabbit
Release Notes