NETOBSERV-2608: TLS metrics#2707
Conversation
Create specific metrics for TLS, counting flow, labelled by: - TLSVersion - TLSCipherSuite - TLSGroup - TLSTypes - Proto
|
Skipping CI for Draft Pull Request. |
|
@jotak: This pull request references NETOBSERV-2608 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughAdds TLS flow metrics and TLSTracking gating, extends CRD/type enums and defaults, updates core metric definitions and TLS charts, adds OTEL TLS mappings, renames frontend TLSCurve→TLSGroup, enables TLSTracking in test specs, and extends Makefile/CSV handling for a PF5 console-plugin image variant. ChangesPlugin Image PF5 Variant Support
TLS Metrics Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2707 +/- ##
==========================================
- Coverage 72.48% 72.40% -0.09%
==========================================
Files 107 107
Lines 11482 11479 -3
==========================================
- Hits 8323 8311 -12
- Misses 2659 2667 +8
- Partials 500 501 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/ok-to-test |
openshift-ci
Bot
added
the
ok-to-test
|
New images: quay.io/netobserv/network-observability-operator:cdee50e
quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-cdee50e
quay.io/netobserv/network-observability-operator-catalog:v0.0.0-sha-cdee50eThey will expire in two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:cdee50e make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-cdee50eOr as a Catalog Source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-sha-cdee50e
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
|
New changes are detected. LGTM label has been removed. |
github-actions
Bot
removed
the
ok-to-test
jotak
added
the
ok-to-test
|
New images: quay.io/netobserv/network-observability-operator:7583751
quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-7583751
quay.io/netobserv/network-observability-operator-catalog:v0.0.0-sha-7583751They will expire in two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:7583751 make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-7583751Or as a Catalog Source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-sha-7583751
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/pkg/dashboards/dashboard_test.go (1)
90-170:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an edge-case test for TLS chart generation.
The new TLS assertions cover happy-path titles only. Please add one test where TLS metrics are absent/empty and assert TLS panels are not generated (or are handled deterministically), so chart selection regressions are caught.
As per coding guidelines
**/*_test.go: include tests for valid cases, invalid input, and edge cases (empty, nil values).🤖 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 `@internal/pkg/dashboards/dashboard_test.go` around lines 90 - 170, The test TestCreateFlowMetricsDashboard_DefaultList only asserts TLS happy-path titles; add an edge-case unit test that simulates missing/empty TLS metrics by calling CreateFlowMetricsDashboards with a metrics.Definitions set that has TLS metrics removed (e.g., manipulate the result from metrics.GetDefinitions(util.SpecForMetrics(), false) to clear TLS-related entries) and then parse the dashboard JSON via FromBytes and assert that d.FindRow("TLS") either returns nil/empty or that its Titles() is empty (deterministic behavior), ensuring TLS panels are not generated when TLS metrics are absent.internal/pkg/metrics/predefined_metrics.go (1)
130-143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse one family tag for both drop counters.
ignoreTagsmatching is exact. The packets metric uses"drops", but the bytes metric uses"drop", so disabling the drop family will still leave*_drop_bytes_totalenabled.Suggested fix
- tags: []string{group, tagBytes, "drop"}, + tags: []string{group, tagBytes, "drops"},🤖 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 `@internal/pkg/metrics/predefined_metrics.go` around lines 130 - 143, The bytes drop metric added to predefinedMetrics uses an inconsistent family tag ("drop") compared to the packets drop metric ("drops"), so ignoreTags won't match; update the taggedMetricDefinition for the bytes metric (the entry constructing metricslatest.FlowMetricSpec within predefinedMetrics) to use the same family tag as the packets counter (make the tags slice contain "drops" instead of "drop" or otherwise align both metrics to the same family tag) so disabling the drop family via ignoreTags correctly disables both *_drop_bytes_total and *_drops_* counters.
🤖 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.
Inline comments:
In `@internal/pkg/metrics/predefined_charts.go`:
- Line 257: The TLS percentage denominator uses a hardcoded OR of
namespace/workload/node totals which yields the leftmost match in mixed-scope
environments; update tlsStatusChart() to accept an extra parameter (e.g.,
matchingTotalMetric string) and use that parameter in the PromQL denominator
instead of the OR expression, then update every call site that constructs TLS
metric charts to pass the appropriate matching total metric name
(namespace_flows_total, workload_flows_total or node_flows_total) so each
scope’s TLS numerator divides by its own total.
In `@internal/pkg/metrics/predefined_metrics.go`:
- Around line 24-26: The change removed the TLSVersion label from the shared
label slices (tagNodes, tagNamespaces, tagWorkloads) which silently mutates the
existing node_*, namespace_*, and workload_* metric schemas; revert or preserve
the legacy TLSVersion label in those slices to avoid breaking existing
PromQL/recording rules/dashboards, and if you need a TLS-specific breakdown
instead, add new metric names (or new dedicated label slices and metric
definitions) that include TLSVersion rather than removing it from
tagNodes/tagNamespaces/tagWorkloads so existing metrics remain backwards
compatible.
---
Outside diff comments:
In `@internal/pkg/dashboards/dashboard_test.go`:
- Around line 90-170: The test TestCreateFlowMetricsDashboard_DefaultList only
asserts TLS happy-path titles; add an edge-case unit test that simulates
missing/empty TLS metrics by calling CreateFlowMetricsDashboards with a
metrics.Definitions set that has TLS metrics removed (e.g., manipulate the
result from metrics.GetDefinitions(util.SpecForMetrics(), false) to clear
TLS-related entries) and then parse the dashboard JSON via FromBytes and assert
that d.FindRow("TLS") either returns nil/empty or that its Titles() is empty
(deterministic behavior), ensuring TLS panels are not generated when TLS metrics
are absent.
In `@internal/pkg/metrics/predefined_metrics.go`:
- Around line 130-143: The bytes drop metric added to predefinedMetrics uses an
inconsistent family tag ("drop") compared to the packets drop metric ("drops"),
so ignoreTags won't match; update the taggedMetricDefinition for the bytes
metric (the entry constructing metricslatest.FlowMetricSpec within
predefinedMetrics) to use the same family tag as the packets counter (make the
tags slice contain "drops" instead of "drop" or otherwise align both metrics to
the same family tag) so disabling the drop family via ignoreTags correctly
disables both *_drop_bytes_total and *_drops_* counters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 56bc636b-716d-4e2b-b8b1-75834d8c073f
📒 Files selected for processing (17)
.mk/development.mkapi/flowcollector/v1beta2/flowcollector_alert_types.goapi/flowcollector/v1beta2/flowcollector_defaults.goapi/flowcollector/v1beta2/flowcollector_types.gobundle/manifests/flows.netobserv.io_flowcollectors.yamlconfig/crd/bases/flows.netobserv.io_flowcollectors.yamldocs/FlowCollector.mdhelm/crds/flows.netobserv.io_flowcollectors.yamlinternal/controller/consoleplugin/config/static-frontend-config.yamlinternal/controller/flp/flp_pipeline_builder_test.gointernal/pkg/dashboards/dashboard_test.gointernal/pkg/helper/otel/otel-config.jsoninternal/pkg/helper/otel/otel_config_test.gointernal/pkg/metrics/predefined_charts.gointernal/pkg/metrics/predefined_metrics.gointernal/pkg/metrics/predefined_metrics_test.gointernal/pkg/test/util/utils.go
| Unit: metricslatest.UnitPercent, | ||
| Queries: []metricslatest.Query{ | ||
| { | ||
| PromQL: `sum(rate($METRIC[2m])) / (sum(rate(netobserv_namespace_flows_total[2m])) OR sum(rate(netobserv_workload_flows_total[2m])) OR sum(rate(netobserv_node_flows_total[2m])))`, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify tlsStatusChart call sites and check whether mixed totals can coexist.
set -euo pipefail
echo "== tlsStatusChart call sites =="
rg -n --type=go '\btlsStatusChart\s*\('
echo
echo "== default include lists containing flow totals =="
rg -n --type=go '"(namespace_flows_total|workload_flows_total|node_flows_total|namespace_tls_flows_total|workload_tls_flows_total|node_tls_flows_total)"'Repository: netobserv/netobserv-operator
Length of output: 5095
🏁 Script executed:
#!/bin/bash
# Examine the tlsStatusChart function and its call site context
echo "== tlsStatusChart function definition (lines 240-290) =="
sed -n '240,290p' internal/pkg/metrics/predefined_charts.go
echo
echo "== Call site in predefined_metrics.go (lines 200-220) =="
sed -n '200,220p' internal/pkg/metrics/predefined_metrics.goRepository: netobserv/netobserv-operator
Length of output: 2429
🏁 Script executed:
#!/bin/bash
# Check how TLS metrics relate to flow metrics - are they paired?
echo "== TLS metric definitions =="
rg -A 3 'tls.*_total' internal/pkg/metrics/predefined_metrics.go | head -30
echo
echo "== How namespace/workload/node metrics are structured =="
rg -B 2 -A 5 'tlsStatusChart\(\)' internal/pkg/metrics/predefined_metrics.goRepository: netobserv/netobserv-operator
Length of output: 661
🏁 Script executed:
#!/bin/bash
# Find where $METRIC gets substituted/replaced in the code
echo "== $METRIC substitution =="
rg -n '\$METRIC' internal/pkg/metrics/
echo
echo "== Check if tests mix multiple metric scopes =="
rg -B 2 -A 2 'GetDefinitions.*node.*namespace.*workload' internal/pkg/metrics/predefined_metrics_test.goRepository: netobserv/netobserv-operator
Length of output: 3225
🏁 Script executed:
#!/bin/bash
# Look at how charts are used - does replacement happen?
echo "== Chart Query usage and replacement =="
rg -n 'Query.*PromQL\|PromQL.*\$METRIC' internal/pkg/metrics/ | head -20
echo
echo "== Monitor or dashboard code that processes these charts =="
rg -l 'Chart\|PromQL' internal/controller/monitoring/ internal/pkg/ | head -5Repository: netobserv/netobserv-operator
Length of output: 173
🏁 Script executed:
#!/bin/bash
# Search for where charts are processed - look in monitoring controller
echo "== monitoring_controller.go - full scan for Chart handling =="
rg -n 'Chart|PromQL' internal/controller/monitoring/monitoring_controller.go | head -30
echo
echo "== Search for $METRIC replacement in codebase =="
rg -n 'Replace.*\$METRIC\|strings.Replace.*METRIC' internal/
echo
echo "== Look for Sprintf or formatting of PromQL =="
rg -n 'fmt.Sprintf.*PromQL\|Sprintf.*%' internal/pkg/metrics/ | head -20Repository: netobserv/netobserv-operator
Length of output: 225
🏁 Script executed:
#!/bin/bash
# Check if there's a dashboard/query builder that handles substitution
echo "== Files that import or use predefined_charts =="
rg -l 'predefined_charts\|tlsStatusChart' internal/
echo
echo "== Check FlowMetricSpec usage and how charts are stored =="
rg -B 3 -A 3 'Charts:.*\[\]' internal/pkg/metrics/predefined_metrics.go | head -40Repository: netobserv/netobserv-operator
Length of output: 181
🏁 Script executed:
#!/bin/bash
# Read the full tlsStatusChart function and surrounding context
echo "== Full tlsStatusChart and context (lines 246-286) =="
sed -n '246,286p' internal/pkg/metrics/predefined_charts.go
echo
echo "== Examine how metrics are built and charts assigned (lines 195-225) =="
sed -n '195,225p' internal/pkg/metrics/predefined_metrics.go
echo
echo "== Check MetricName vs Chart relationship =="
sed -n '200,215p' internal/pkg/metrics/predefined_metrics.go | cat -ARepository: netobserv/netobserv-operator
Length of output: 3433
🏁 Script executed:
#!/bin/bash
# Look for where charts might be processed - check for any replacement/substitution
echo "== All files importing FlowMetricSpec or Chart types =="
fd -e go -x grep -l 'FlowMetricSpec\|metricslatest\.Chart' {} \; 2>/dev/null | head -10
echo
echo "== Search for where metric definitions are consumed =="
rg 'GetDefinitions\|predefinedMetrics' --type go -lRepository: netobserv/netobserv-operator
Length of output: 636
🏁 Script executed:
#!/bin/bash
# Check dashboard.go to see how charts are rendered/processed
echo "== dashboard.go - search for METRIC substitution or chart processing =="
grep -n 'METRIC\|PromQL\|Chart' internal/pkg/dashboards/dashboard.go | head -40
echo
echo "== helper.go - might have utilities for chart/metric processing =="
cat -n internal/pkg/metrics/helper.go | head -50Repository: netobserv/netobserv-operator
Length of output: 1518
🏁 Script executed:
#!/bin/bash
# Check if MetricName from FlowMetricSpec connects to chart rendering
echo "== Examine dashboard.go structure =="
head -100 internal/pkg/dashboards/dashboard.go | tail -80
echo
echo "== Search for where $METRIC or placeholders are replaced =="
rg -C 3 'strings\.Replace|Sprintf.*METRIC|METRIC.*Replace' internal/pkg/dashboards/Repository: netobserv/netobserv-operator
Length of output: 4098
TLS traffic percentage will be incorrect in mixed-scope configurations.
The denominator at line 257 is hardcoded to (netobserv_namespace_flows_total OR netobserv_workload_flows_total OR netobserv_node_flows_total), but the numerator is dynamically substituted per metric. When multiple scopes exist, PromQL OR returns the leftmost match, causing the workload and node TLS charts to divide by namespace totals instead of their own scope.
Pass the matching flow metric name to tlsStatusChart() and use it in the denominator instead of hardcoded OR logic:
Proposed direction
-func tlsStatusChart() []metricslatest.Chart {
+func tlsStatusChart(totalFlowsMetric string) []metricslatest.Chart {
...
- PromQL: `sum(rate($METRIC[2m])) / (sum(rate(netobserv_namespace_flows_total[2m])) OR sum(rate(netobserv_workload_flows_total[2m])) OR sum(rate(netobserv_node_flows_total[2m])))`,
+ PromQL: fmt.Sprintf(`sum(rate($METRIC[2m])) / sum(rate(%s[2m]))`, totalFlowsMetric),
...
}Then pass the matching total metric (namespace_flows_total, workload_flows_total, or node_flows_total) from each TLS metric definition call site.
🤖 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 `@internal/pkg/metrics/predefined_charts.go` at line 257, The TLS percentage
denominator uses a hardcoded OR of namespace/workload/node totals which yields
the leftmost match in mixed-scope environments; update tlsStatusChart() to
accept an extra parameter (e.g., matchingTotalMetric string) and use that
parameter in the PromQL denominator instead of the OR expression, then update
every call site that constructs TLS metric charts to pass the appropriate
matching total metric name (namespace_flows_total, workload_flows_total or
node_flows_total) so each scope’s TLS numerator divides by its own total.
There was a problem hiding this comment.
the namespace/workload/node variants differ by their aggregation keys but they should all be identical when not aggregated by key. That's what we do here: sum(...) and not sum by (...)(...). Their sum is expected to be equal.
There was a problem hiding this comment.
@jotak, you're right. Since the query uses sum(...) with no by (...) clause, the result collapses to a global scalar — and all three metrics represent the same underlying flows just with different label dimensions, so their fully-aggregated sums are equal. The OR is just a safety net for whichever metric happens to be present. My concern was mistaken.
The metrics tagging system is a leftover from old API, and is dead code since that API was removed.
github-actions
Bot
removed
the
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@internal/pkg/metrics/predefined_metrics.go`:
- Around line 162-176: The tlsStatusChart helper is missing the group argument
so all TLS metrics use identical chart variants; update the loop that builds
FlowMetricSpec (using group, groupTrimmed, labels, tlsLabels, predefinedMetrics)
to call tlsStatusChart(group) instead of tlsStatusChart() so chartVariantsFor
can generate group-specific variants for tagNodes/tagNamespaces/tagWorkloads
(ensuring namespace/workload "infra"/"app" and node-specific configs are
produced).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc3fb3b9-5707-4b36-88be-3fc21f6d257f
📒 Files selected for processing (2)
internal/pkg/metrics/predefined_metrics.gointernal/pkg/metrics/predefined_metrics_test.go
💤 Files with no reviewable changes (1)
- internal/pkg/metrics/predefined_metrics_test.go
| // TLS | ||
| for _, group := range []string{tagNodes, tagNamespaces, tagWorkloads} { | ||
| groupTrimmed := strings.TrimSuffix(group, "s") | ||
| labels := mapLabels[group] | ||
| tlsLabels := labels | ||
| tlsLabels = append(tlsLabels, "TLSVersion", "TLSCipherSuite", "TLSGroup") | ||
| predefinedMetrics = append(predefinedMetrics, metricslatest.FlowMetricSpec{ | ||
| MetricName: fmt.Sprintf("%s_tls_flows_total", groupTrimmed), | ||
| Type: metricslatest.CounterMetric, | ||
| Help: fmt.Sprintf("Total TLS flows per %s", groupTrimmed), | ||
| Filters: []metricslatest.MetricFilter{{Field: "TLSTypes", MatchType: metricslatest.MatchPresence}}, | ||
| Labels: tlsLabels, | ||
| Charts: tlsStatusChart(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the tlsStatusChart and ipsecStatusChart definitions and compare signatures
rg -n "func tlsStatusChart\|func ipsecStatusChart" --type go -A 5Repository: netobserv/netobserv-operator
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Try broader search patterns for chart functions
echo "=== Searching for tlsStatusChart definition ==="
rg -n "tlsStatusChart" --type go -A 3 -B 1
echo ""
echo "=== Searching for ipsecStatusChart definition ==="
rg -n "ipsecStatusChart" --type go -A 3 -B 1
echo ""
echo "=== Searching for other chart helpers ==="
rg -n "rttCharts|dropCharts|dnsCharts|netpolCharts" --type go -A 2 -B 1Repository: netobserv/netobserv-operator
Length of output: 4157
🏁 Script executed:
#!/bin/bash
# Check the tlsStatusChart implementation
echo "=== tlsStatusChart implementation ==="
sed -n '246,280p' internal/pkg/metrics/predefined_charts.go
echo ""
echo "=== ipsecStatusChart implementation (for comparison) ==="
sed -n '218,245p' internal/pkg/metrics/predefined_charts.goRepository: netobserv/netobserv-operator
Length of output: 2181
🏁 Script executed:
#!/bin/bash
# Check what chartVariantsFor does and how it uses the group parameter
echo "=== Search for chartVariantsFor definition ==="
rg -n "func chartVariantsFor" --type go -A 10Repository: netobserv/netobserv-operator
Length of output: 1182
🏁 Script executed:
#!/bin/bash
# Get the full chartVariantsFor function
sed -n '288,330p' internal/pkg/metrics/predefined_charts.goRepository: netobserv/netobserv-operator
Length of output: 1666
Pass group to tlsStatusChart() to generate proper chart variants for each metric type.
tlsStatusChart() is the only chart helper lacking a group argument. All others—rttCharts(group), dropCharts(group, ...), dnsCharts(group), netpolCharts(group), ipsecStatusChart(group)—receive it so chartVariantsFor() can generate group-specific variants (namespace/workload "infra" and "app" layers, node-specific configs). Without it, all three group types get identical charts, breaking dashboard configurations for namespace and workload metrics.
Diff
- Charts: tlsStatusChart(),
+ Charts: tlsStatusChart(group),🤖 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 `@internal/pkg/metrics/predefined_metrics.go` around lines 162 - 176, The
tlsStatusChart helper is missing the group argument so all TLS metrics use
identical chart variants; update the loop that builds FlowMetricSpec (using
group, groupTrimmed, labels, tlsLabels, predefinedMetrics) to call
tlsStatusChart(group) instead of tlsStatusChart() so chartVariantsFor can
generate group-specific variants for tagNodes/tagNamespaces/tagWorkloads
(ensuring namespace/workload "infra"/"app" and node-specific configs are
produced).
There was a problem hiding this comment.
not doing so because the charts are not displayed per group in this case (yes, it's a new pattern)
Charts are deduplicated in createFlowMetricsDashboard so it won't create multiple instances of the same
There was a problem hiding this comment.
@jotak Got it — the TLS charts are intentionally group-agnostic, relying on deduplication in createFlowMetricsDashboard rather than per-group variants. Makes sense as a new pattern.
( ˙ ɥ ˙ ) ~ learning noted
✏️ Learnings added
Learnt from: jotak
Repo: netobserv/netobserv-operator PR: 2707
File: internal/pkg/metrics/predefined_metrics.go:162-176
Timestamp: 2026-05-07T12:45:12.308Z
Learning: In `internal/pkg/metrics/predefined_metrics.go` (netobserv/netobserv-operator), `tlsStatusChart()` intentionally takes no `group` argument, unlike other chart helpers (e.g. `ipsecStatusChart(group)`, `rttCharts(group)`). TLS charts are not displayed per group; deduplication in `createFlowMetricsDashboard` prevents duplicate chart instances, so passing `group` is unnecessary and undesirable for this metric type.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
jotak
added
the
ok-to-test
|
New images: quay.io/netobserv/network-observability-operator:cfbe85d
quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-cfbe85d
quay.io/netobserv/network-observability-operator-catalog:v0.0.0-sha-cfbe85dThey will expire in two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:cfbe85d make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-cfbe85dOr as a Catalog Source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-sha-cfbe85d
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
Amoghrd
added
ok-to-test
4 participants
Description
Create specific metrics for TLS, counting flow, labelled by:
Add related charts in the monitoring dashboard
Add related otel mapping
Dependencies
Checklist
Summary by CodeRabbit
New Features
UI
Documentation
Chores