Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NETOBSERV-1005 dashboard: use sum...by #356

Merged
merged 3 commits into from
Jun 2, 2023

Conversation

jotak
Copy link
Member

@jotak jotak commented Jun 1, 2023

  • Fix metrics aggregations by using sum...by
  • Do not split by FLP pod reporter as it's an implementation detail

1.3 backport: #358

- Fix metrics aggregations by using sum...by
- Do not split by FLP pod reporter as it's an implementation detail
OlivierCazade
OlivierCazade previously approved these changes Jun 1, 2023
Copy link
Contributor

@OlivierCazade OlivierCazade left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Jun 1, 2023

Codecov Report

Merging #356 (c218156) into main (ca61fad) will increase coverage by 0.52%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #356      +/-   ##
==========================================
+ Coverage   53.37%   53.90%   +0.52%     
==========================================
  Files          45       45              
  Lines        5328     5328              
==========================================
+ Hits         2844     2872      +28     
+ Misses       2281     2256      -25     
+ Partials      203      200       -3     
Flag Coverage Δ
unittests 53.90% <ø> (+0.52%) ⬆️

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

see 6 files with indirect coverage changes

jpinsonneau
jpinsonneau previously approved these changes Jun 1, 2023
@memodi
Copy link
Contributor

memodi commented Jun 1, 2023

/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 Jun 1, 2023
@github-actions
Copy link

github-actions bot commented Jun 1, 2023

New images:

  • quay.io/netobserv/network-observability-operator:f46fd7e
  • quay.io/netobserv/network-observability-operator-bundle:v0.0.0-f46fd7e
  • quay.io/netobserv/network-observability-operator-catalog:v0.0.0-f46fd7e

They will expire after two weeks.

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

@memodi
Copy link
Contributor

memodi commented Jun 1, 2023

@jotak this overall looks good to me. It would have been nice to have it limited to Top 5 with topK query but based on some reading it looks like when plotting w/ graphs that's not possible. Grafana has some features but probably OCP console Dashboards doesn't have it.

Also, could we do something about those empty parentheses? quick search online I didn't find any straightforward solution to handle this in prometheus query. For our netflow-traffic page, we do that by replacing it with non-pods I guess that's done in console-plugin.

image

So if no further changes are being added to this PR treat this as /label qe-approved and feel to free to merge it.

@jotak
Copy link
Member Author

jotak commented Jun 2, 2023

@memodi there's probably something we can do with label_replace , I'll make a try

@jotak jotak dismissed stale reviews from jpinsonneau and OlivierCazade via 9a2cc85 June 2, 2023 09:27
@openshift-ci openshift-ci bot removed the lgtm label Jun 2, 2023
@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jun 2, 2023
@jotak jotak added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jun 2, 2023
@jotak
Copy link
Member Author

jotak commented Jun 2, 2023

@memodi so my last commit should address your second comment. On the first one (the top5 thing), yeah I didn't find a way to force restricting 5 outputs, and anyway I thought having a bit more than 5 could be interesting, hence I just removed the "Top 5" mention from the title

@github-actions
Copy link

github-actions bot commented Jun 2, 2023

New images:

  • quay.io/netobserv/network-observability-operator:c240674
  • quay.io/netobserv/network-observability-operator-bundle:v0.0.0-c240674
  • quay.io/netobserv/network-observability-operator-catalog:v0.0.0-c240674

They will expire after two weeks.

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

@jotak
Copy link
Member Author

jotak commented Jun 2, 2023

let me try one last thing: I'd like to remove the node-to-node or external-to-node traffic from the workloads-kind of metrics

@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jun 2, 2023
@jotak jotak added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jun 2, 2023
@jotak
Copy link
Member Author

jotak commented Jun 2, 2023

Ok, I've modified the "workloads" graph to exclude node-to-node traffic, so it's not overlapping with the other graph dedicated to nodes

Capture d’écran du 2023-06-02 12-06-07

wdyt?

@github-actions
Copy link

github-actions bot commented Jun 2, 2023

New images:

  • quay.io/netobserv/network-observability-operator:79f0c6f
  • quay.io/netobserv/network-observability-operator-bundle:v0.0.0-79f0c6f
  • quay.io/netobserv/network-observability-operator-catalog:v0.0.0-79f0c6f

They will expire after two weeks.

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

@memodi
Copy link
Contributor

memodi commented Jun 2, 2023

wdyt?

I am yet to try, but this looks great :D , thank you!

@memodi
Copy link
Contributor

memodi commented Jun 2, 2023

/lgtm
/label qe-approved

@openshift-ci openshift-ci bot added qe-approved QE has approved this pull request lgtm labels Jun 2, 2023
@jotak
Copy link
Member Author

jotak commented Jun 2, 2023

thanks!
/approve
There's the backporting PR then to /lgtm: #358

@openshift-ci
Copy link

openshift-ci bot commented Jun 2, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jotak

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

The pull request process is described here

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

@openshift-ci openshift-ci bot added the approved label Jun 2, 2023
@openshift-merge-robot openshift-merge-robot merged commit 0e4dfd1 into netobserv:main Jun 2, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. qe-approved QE has approved this pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants