Skip to content

NETOBSERV-2655: additionalIncludeList field to append metrics to default#2546

Open
Amoghrd wants to merge 1 commit intonetobserv:mainfrom
Amoghrd:append-includeList
Open

NETOBSERV-2655: additionalIncludeList field to append metrics to default#2546
Amoghrd wants to merge 1 commit intonetobserv:mainfrom
Amoghrd:append-includeList

Conversation

@Amoghrd
Copy link
Member

@Amoghrd Amoghrd commented Mar 13, 2026

Description

Adds a new additionalIncludeList field to allow appending metrics to the defaults without breaking backward compatibility.

Problem: The original includeList field overwrites the default metrics list. Users who want to add a few extra metrics need to specify all defaults + their additions, which is cumbersome.

Solution:

  • Keep includeList behavior unchanged (overwrites defaults) - backwards compatible
  • Add new additionalIncludeList field that appends to defaults
  • Fields are mutually exclusive - if both are set, includeList takes precedence and a validation warning is shown

Usage Examples:

Append to defaults:

spec:
  processor:
    metrics:
      additionalIncludeList:
        - namespace_egress_bytes_total
        - namespace_ingress_bytes_total

Override defaults (existing behavior):

spec:
  processor:
    metrics:
      includeList:
        - node_ingress_bytes_total

Changes

  • Added additionalIncludeList field to FLPMetrics API
  • Updated GetIncludeList() to support the new field
  • Added validation to warn when both fields are set
  • Added comprehensive tests for new functionality
  • Updated CRDs, bundle, and documentation

Dependencies

n/a

Checklist

  • Does the changes in PR need specific configuration or environment set up for testing?
    • if so please describe it in PR description.
  • I have added thorough unit tests for the change.
  • QE requirements (check 1 from the list):
    • Standard QE validation, with pre-merge tests unless stated otherwise.
    • Regression tests only (e.g. refactoring with no user-facing change).
    • No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Mar 13, 2026

@Amoghrd: This pull request references NETOBSERV-2655 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 "4.22.0" version, but no target version was set.

Details

In response to this:

Description

Instead of overwriting metrics when added to includeList, now we append them to the default list.
Also updated the tests based on these changes

Dependencies

n/a

Checklist

  • Does the changes in PR need specific configuration or environment set up for testing?
    • if so please describe it in PR description.
  • I have added thorough unit tests for the change.
  • QE requirements (check 1 from the list):
  • Standard QE validation, with pre-merge tests unless stated otherwise.
  • Regression tests only (e.g. refactoring with no user-facing change).
  • No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

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.

@Amoghrd Amoghrd force-pushed the append-includeList branch 2 times, most recently from 4221dd0 to 735d68e Compare March 13, 2026 15:09
@Amoghrd Amoghrd requested a review from jotak March 13, 2026 15:10
@Amoghrd Amoghrd force-pushed the append-includeList branch 2 times, most recently from e341726 to 2999d48 Compare March 13, 2026 15:24
@codecov
Copy link

codecov bot commented Mar 13, 2026

Codecov Report

❌ Patch coverage is 66.66667% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.11%. Comparing base (3a14443) to head (8f898d8).
⚠️ Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
api/flowcollector/v1beta2/zz_generated.deepcopy.go 0.00% 6 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2546      +/-   ##
==========================================
- Coverage   72.14%   72.11%   -0.03%     
==========================================
  Files         105      105              
  Lines       10762    10783      +21     
==========================================
+ Hits         7764     7776      +12     
- Misses       2517     2525       +8     
- Partials      481      482       +1     
Flag Coverage Δ
unittests 72.11% <66.66%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...flowcollector/v1beta2/flowcollector_alert_types.go 97.72% <100.00%> (+0.13%) ⬆️
api/flowcollector/v1beta2/flowcollector_types.go 100.00% <ø> (ø)
...lector/v1beta2/flowcollector_validation_webhook.go 72.83% <100.00%> (+0.74%) ⬆️
api/flowcollector/v1beta2/zz_generated.deepcopy.go 35.58% <0.00%> (-0.26%) ⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@jotak jotak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't change the behaviour of the existing API, it would be a breaking change.
We need instead to create a new setting, such as "additionalIncludeList", and keep the existing as it is.

@Amoghrd
Copy link
Member Author

Amoghrd commented Mar 16, 2026

Sounds good, will do that

@Amoghrd Amoghrd force-pushed the append-includeList branch from 2999d48 to c089707 Compare March 16, 2026 14:04
@openshift-ci
Copy link

openshift-ci bot commented Mar 16, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jotak. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Amoghrd Amoghrd force-pushed the append-includeList branch from c089707 to e970bd3 Compare March 16, 2026 14:07
@openshift-ci-robot
Copy link
Collaborator

@Amoghrd: This pull request references NETOBSERV-2655 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 "4.22.0" version, but no target version was set.

Details

In response to this:

@/tmp/pr_body.md

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.

@Amoghrd Amoghrd changed the title NETOBSERV-2655: Append includeList NETOBSERV-2655: additionalIncludeList field to append metrics to default Mar 16, 2026
Copy link
Member

@jotak jotak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a small comment, LGTM otherwise

Copy link
Member

@jotak jotak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Amoghrd , lgtm

@openshift-ci openshift-ci bot added the lgtm label Mar 18, 2026
@oliver-smakal
Copy link

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Mar 26, 2026
@github-actions
Copy link

New images:

quay.io/netobserv/network-observability-operator:aa88674
quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-aa88674
quay.io/netobserv/network-observability-operator-catalog:v0.0.0-sha-aa88674

They will expire in two weeks.

To deploy this build:

# Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:aa88674 make deploy

# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-aa88674

Or 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-aa88674
  displayName: NetObserv development catalog
  publisher: Me
  updateStrategy:
    registryPoll:
      interval: 1m

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jira/valid-reference lgtm ok-to-test To set manually when a PR is safe to test. Triggers image build on PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants