[c-kruse]

Closes #2609

Summary by CodeRabbit

• New Features
  • Listener responses now include routing model, routing-key count, strategy, and optional strategy value.
  • Service responses now report the number of multi-key listeners.
• Bug Fixes
  • Explicit zero values for supported fields are now preserved in encoded data rather than omitted.

[coderabbitai]

📝 Walkthrough

The change adds strategy and routing details to Vanflow listener records and Network Observer API responses. It also reports multi-key listener counts for services and preserves explicitly supplied zero values during encoding and store patches.

Changes: Listener routing data

Layer / File(s)	Summary
Vanflow strategy attributes pkg/vanflow/records.go, pkg/vanflow/encoding/*, pkg/vanflow/store/syncmap_test.go	Listener records add strategy name and value fields. Pointer encoding preserves explicitly supplied zero values. Tests cover encoding and patch behavior.
Listener and service API fields cmd/network-observer/internal/api/types_gen.go, cmd/network-observer/spec/openapi.yaml	The API types and schema define listener strategy, routing model, routing-key count, strategy value, and the service multi-key listener count.
Listener response mapping cmd/network-observer/internal/server/views/views.go, cmd/network-observer/internal/server/*_test.go	Listener conversion maps strategy values and counts routing keys among sibling listeners. Service conversion populates the multi-key listener count. Tests cover list and detail responses, ended listeners, and service counts.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant VanflowStore
  participant NewListenerProvider
  participant NetworkObserverAPI
  VanflowStore->>NewListenerProvider: listener and sibling records
  NewListenerProvider->>NetworkObserverAPI: mapped listener routing fields

Loading

Merge Risk: 🔵 Low · up to 030a1

Services can report an ended multi-key listener as current. Correct the count before merging, or accept the bounded reporting discrepancy.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 8 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly identifies the primary change: exposing MultiKeyListener-related attributes in the Network Observer API. This matches the API schema, listener records, service records, and supportin…
Linked Issues check		[#2609] The issue asks for the LISTENER attributes strategy and strategy value in the Network Observer API, so the console can identify listeners that belong to a multi-address tcpListener. The wh…
Out of Scope Changes check		The PR changes stay within the issue scope. The core changes add the strategy and strategyValue attributes to the vanflow record, the generated API types, the OpenAPI spec, and the view conversion…

Full details: Title check

Explanation

The title clearly identifies the primary change: exposing MultiKeyListener-related attributes in the Network Observer API. This matches the API schema, listener records, service records, and supporting behavior added by the pull request.

Full details: Linked Issues check

Explanation

[#2609] The issue asks for the LISTENER attributes strategy and strategy value in the Network Observer API, so the console can identify listeners that belong to a multi-address tcpListener. The whole-PR summary shows this implementation at head 030a187: (1) cmd/network-observer/internal/api/types_gen.go adds ListenerStrategyType (not-applicable, priority, weighted) and ListenerRecord.Strategy and ListenerRecord.StrategyValue *uint64; (2) cmd/network-observer/spec/openapi.yaml defines strategy and integer strategyValue on ListenerRecord; (3) pkg/vanflow/records.go adds StrategyName and StrategyValue (tags 68 and 69) so the values arrive from the router; (4) cmd/network-observer/internal/server/views/views.go maps strategy names case-insensitively and copies the strategy value into the API record. pkg/vanflow/encoding/encoder.go also preserves explicit zero values, so a strategy value of 0 is not dropped. Automated tests cover the zero-value encoding (encoder_test.go), the store patch behavior for StrategyValue (syncmap_test.go), and a weighted fixture in address_test.go. The visible API test TestListenerRoutingKeyCounts checks routingKeyCount only. No test visible in the summary checks the strategy and strategyValue fields in list or detail API responses. The requirement is implemented, but the API-level test coverage of these two fields is not confirmed.

Full details: Out of Scope Changes check

Explanation

The PR changes stay within the issue scope. The core changes add the strategy and strategyValue attributes to the vanflow record, the generated API types, the OpenAPI spec, and the view conversion. The encoder fix (pointerEncoder.encode for rawFieldEncoder) and its tests support the strategy value attribute, because an explicit 0 must survive encoding. The syncmap_test.go test supports the same zero-value handling. The extra fields routingModel, routingKeyCount, and ServiceRecord.multiKeyListenerCount derive from the multi-key listener concept. They help the console identify listeners in a multi-address tcpListener, which is the purpose of #2609. The issue does not name these fields, but it does not exclude them. The summary shows no unrelated change. The reviewed evidence does not show a concrete unrelated change. Whether routingKeyCount and multiKeyListenerCount are required by the console is a product decision that the issue does not state.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 8 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments: 1

cmd/network-observer/internal/server/listener_test.go: 1

54-89: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Assert Strategy and StrategyValue in the listener responses.

The fixture sets StrategyName to "weighted" and StrategyValue to 0, but the test checks only RoutingKeyCount. A regression in NewListenerProvider can therefore remove or alter either API field without failing this test. Assert the weighted strategy and the non-nil zero value for the list and detail responses.

Suggested fix

 import (
 	"context"
 	"log/slog"
 	"testing"
 	"time"

+	"github.com/skupperproject/skupper/cmd/network-observer/internal/api"
 	"github.com/skupperproject/skupper/cmd/network-observer/internal/collector"
 	"github.com/skupperproject/skupper/pkg/vanflow"
 	"github.com/skupperproject/skupper/pkg/vanflow/store"
@@
 			counts := map[string]int{}
 			for _, result := range response.JSON200.Results {
 				counts[result.Identity] = result.RoutingKeyCount
+				if result.Identity == "single" {
+					assert.Equal(t, result.Strategy, api.NotApplicable)
+					assert.Assert(t, result.StrategyValue == nil)
+				} else {
+					assert.Equal(t, result.Strategy, api.Weighted)
+					assert.Assert(t, result.StrategyValue != nil)
+					assert.Equal(t, *result.StrategyValue, uint64(0))
+				}
 			}
 			assert.DeepEqual(t, counts, expected)
 			for _, id := range []string{"first", "ended"} {
 				detail, err := client.ListenerByIDWithResponse(context.Background(), id)
 				assert.NilError(t, err)
 				assert.Equal(t, detail.StatusCode(), 200)
 				assert.Assert(t, detail.JSON200 != nil)
 				assert.Equal(t, detail.JSON200.Results.RoutingKeyCount, expected[id])
+				assert.Equal(t, detail.JSON200.Results.Strategy, api.Weighted)
+				assert.Assert(t, detail.JSON200.Results.StrategyValue != nil)
+				assert.Equal(t, *detail.JSON200.Results.StrategyValue, uint64(0))
 			}

---

ℹ️ Review info

⚙️ Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: d9960fb5-afb7-460b-a976-5ff0434bdca0

📥 Commits

Reviewing files that changed from the base of the PR and between ec41f65 and 030a187.

📒 Files selected for processing: 9

• cmd/network-observer/internal/api/types_gen.go
• cmd/network-observer/internal/server/address_test.go
• cmd/network-observer/internal/server/listener_test.go
• cmd/network-observer/internal/server/views/views.go
• cmd/network-observer/spec/openapi.yaml
• pkg/vanflow/encoding/encoder.go
• pkg/vanflow/encoding/encoder_test.go
• pkg/vanflow/records.go
• pkg/vanflow/store/syncmap_test.go

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Exclude ended listeners from MultiKeyListenerCount.

When a multi-key listener has ended, this loop still counts its record. RoutingKey.Listeners() returns the graph’s listener children without filtering by end time. A service with one active single-key listener and one ended multi-key listener therefore reports multiKeyListenerCount: 1 instead of 0. Apply the end-time check used by the listener routing-key count before incrementing. (raw.githubusercontent.com)

[fgiorgetti]

LGTM. API is returning multiAddress and regular listeners properly.