Skip to content

NETOBSERV-2117 CLI metrics options#289

Merged
jpinsonneau merged 3 commits intonetobserv:mainfrom
jpinsonneau:2117
May 14, 2025
Merged

NETOBSERV-2117 CLI metrics options#289
jpinsonneau merged 3 commits intonetobserv:mainfrom
jpinsonneau:2117

Conversation

@jpinsonneau
Copy link
Member

@jpinsonneau jpinsonneau commented May 7, 2025

Description

  • add includeList option matching partial / full metrics names
  • update metrics and dashboard to allow all cases

Examples:

oc netobserv metrics --include_list=node,workload
oc netobserv metrics --include_list=node_egress_bytes_total,workload_egress_packets_total
oc-netobserv metrics --enable_all --include_list=node,namespace,workload

Output:

opt: include_list, value: node,workload
Matching metrics:
 - node_egress_bytes_total
 - node_ingress_bytes_total
 - workload_egress_bytes_total
 - workload_ingress_bytes_total
 - workload_egress_packets_total
 - workload_ingress_packets_total
 - workload_flows_total
 - workload_drop_packets_total
 - workload_drop_bytes_total

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

  • Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
  • Does this PR require product documentation?
    • If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
  • Does this PR require a product release notes entry?
    • If so, fill in "Release Note Text" in the JIRA.
  • Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
    • If so, make sure it is described in the JIRA ticket.
  • 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).

@jpinsonneau jpinsonneau marked this pull request as draft May 7, 2025 12:44
@jpinsonneau jpinsonneau marked this pull request as ready for review May 7, 2025 13:57
@jpinsonneau jpinsonneau requested review from OlivierCazade and jotak May 7, 2025 13:57
@codecov
Copy link

codecov bot commented May 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 22.60%. Comparing base (9cceb49) to head (30766e4).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #289   +/-   ##
=======================================
  Coverage   22.60%   22.60%           
=======================================
  Files          14       14           
  Lines        1451     1451           
=======================================
  Hits          328      328           
  Misses       1099     1099           
  Partials       24       24           
Flag Coverage Δ
unittests 22.60% <ø> (ø)

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

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

@memodi
Copy link
Member

memodi commented May 8, 2025

/ok-to-test

@github-actions
Copy link

github-actions bot commented May 8, 2025

New image:
quay.io/netobserv/network-observability-cli:977b8d9

It will expire after two weeks.

To use this build, update your commands using:

USER=netobserv VERSION=977b8d9 make commands

or download the updated commands.

@memodi
Copy link
Member

memodi commented May 8, 2025

@jpinsonneau overall this looks good. Intentionally including metrics is definitely a good safety measure. Couple of points:

  • Could we add examples in the PR description in the help as well?
  • I still fear users would forget about that they started these metrics collection and would be probably unclear how to end it, increasing prometheus footprint as a result - I wonder if it makes sense to have similar interface for metrics command as flows/packets with "collector" pod running so when user exists we also cleanup our cli pods which would end metrics collection?

@jpinsonneau
Copy link
Member Author

@jpinsonneau overall this looks good. Intentionally including metrics is definitely a good safety measure. Couple of points:

  • Could we add examples in the PR description in the help as well?

Sure, I can write some example and maybe will make a blog article on top

  • I still fear users would forget about that they started these metrics collection and would be probably unclear how to end it, increasing prometheus footprint as a result - I wonder if it makes sense to have similar interface for metrics command as flows/packets with "collector" pod running so when user exists we also cleanup our cli pods which would end metrics collection?

So the collector will be only responsible of killing the collection ? 🤔
Maybe we could simply implement that in the eBPF agent since it embedds FLP too. That would result as terminating pods and the dashboard will be still accessible until the user cleanup.

We may also rely on jobs that would not restart the pods on completion.

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.

/lgtm

@openshift-ci openshift-ci bot added lgtm and removed lgtm labels May 12, 2025
@openshift-ci
Copy link

openshift-ci bot commented May 12, 2025

New changes are detected. LGTM label has been removed.

@memodi
Copy link
Member

memodi commented May 13, 2025

We may also rely on jobs that would not restart the pods on completion.

definitely jobs seems to fit this use case, I create the follow up story: https://issues.redhat.com/browse/NETOBSERV-2253

@memodi
Copy link
Member

memodi commented May 13, 2025

/label qe-approved

@jpinsonneau
Copy link
Member Author

Rebased without changes

@openshift-ci
Copy link

openshift-ci bot commented May 14, 2025

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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

The pull request process is described 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

@jpinsonneau jpinsonneau merged commit 9ea9deb into netobserv:main May 14, 2025
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants