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-1352 enhance prom filters for RTT metrics #478

Merged
merged 2 commits into from
Oct 9, 2023

Conversation

jpinsonneau
Copy link
Collaborator

@jpinsonneau jpinsonneau commented Aug 29, 2023

This PR improve prom metrics filtering introducing:

  • !nil to match presenceof the field with any value
  • nil to match absence of the field
  • | splitter to match at least one value

@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (914d470) 66.03% compared to head (2f040d3) 67.07%.
Report is 22 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #478      +/-   ##
==========================================
+ Coverage   66.03%   67.07%   +1.04%     
==========================================
  Files          94       94              
  Lines        6921     6724     -197     
==========================================
- Hits         4570     4510      -60     
+ Misses       2092     1948     -144     
- Partials      259      266       +7     
Flag Coverage Δ
unittests 67.07% <82.35%> (+1.04%) ⬆️

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

Files Coverage Δ
pkg/api/encode_prom.go 100.00% <ø> (ø)
pkg/confgen/flowlogs2metrics_config.go 68.83% <100.00%> (ø)
pkg/pipeline/encode/encode_prom.go 77.35% <81.25%> (-0.68%) ⬇️

... and 19 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jotak
jotak previously approved these changes Oct 2, 2023
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 the lgtm label Oct 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 Oct 2, 2023
@github-actions
Copy link

github-actions bot commented Oct 2, 2023

New image:
quay.io/netobserv/flowlogs-pipeline:555ba5d

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=555ba5d make set-flp-image

@jotak
Copy link
Member

jotak commented Oct 2, 2023

@jpinsonneau I flagged LGTM, but taken in the light of bug https://issues.redhat.com/browse/NETOBSERV-1344, we will need to have filters such as "FlowDirection==1 OR FlowDirection==2" and/or "FlowDirection!=0" and/or "FlowDirection~=1|2" ... we should probably think about that before merging

@jpinsonneau
Copy link
Collaborator Author

@jpinsonneau I flagged LGTM, but taken in the light of bug https://issues.redhat.com/browse/NETOBSERV-1344, we will need to have filters such as "FlowDirection==1 OR FlowDirection==2" and/or "FlowDirection!=0" and/or "FlowDirection~=1|2" ... we should probably think about that before merging

Let's just merge all metrics PRs pending before doing the fix everywhere so we don't miss any ?

@openshift-ci openshift-ci bot removed the lgtm label Oct 6, 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 Oct 6, 2023
@jpinsonneau jpinsonneau changed the title NETOBSERV-1231 enhance prom filters for RTT metrics NETOBSERV-1352 enhance prom filters for RTT metrics Oct 6, 2023
@jpinsonneau jpinsonneau requested a review from jotak October 6, 2023 13:34
@jotak jotak added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Oct 9, 2023
@github-actions
Copy link

github-actions bot commented Oct 9, 2023

New image:
quay.io/netobserv/flowlogs-pipeline:e54bae8

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=e54bae8 make set-flp-image

@jotak
Copy link
Member

jotak commented Oct 9, 2023

I'm ok to go with it but just to mention a small concern, it creates some reserved words or characters that are not possible to have in filters as raw text .. even if today it sounds unlikely to have "nil" or "|" actually written in a field and to be willing to filter on that (as a text value), still we are creating reserved keyword that we could avoid by just changing the API.

E.g: as an alternative, we could implement a type param on filters, as an enum: exact (= current behaviour), presence, absence, regex.
wdyt? we could do that as a follow-up

@jpinsonneau
Copy link
Collaborator Author

I'm ok to go with it but just to mention a small concern, it creates some reserved words or characters that are not possible to have in filters as raw text .. even if today it sounds unlikely to have "nil" or "|" actually written in a field and to be willing to filter on that (as a text value), still we are creating reserved keyword that we could avoid by just changing the API.

E.g: as an alternative, we could implement a type param on filters, as an enum: exact (= current behaviour), presence, absence, regex. wdyt? we could do that as a follow-up

As you prefer. I don't have strong opinion on that. Feel free to merge if it's blocking other PRs else I can refactor here; just let me know.

jotak added a commit to jotak/network-observability-operator that referenced this pull request Oct 9, 2023
@jotak
Copy link
Member

jotak commented Oct 9, 2023

/lgtm
I found another reason to enhance the prom API: we need a way to scale up/down the value stored in metrics. We need that for the RTT metric , which currently is expressed as nano-seconds, which isn't convenient when read in dashboards. So, there will be a follow-up

@openshift-ci openshift-ci bot added the lgtm label Oct 9, 2023
@jotak jotak added the no-qe This PR doesn't necessitate QE approval label Oct 9, 2023
@jotak jotak added the no-doc This PR doesn't require documentation change on the NetObserv operator label Oct 9, 2023
@jotak
Copy link
Member

jotak commented Oct 9, 2023

/approve
(this is blocking other PRs)

@openshift-ci
Copy link

openshift-ci bot commented Oct 9, 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 Oct 9, 2023
@openshift-ci openshift-ci bot merged commit 94fca64 into netobserv:main Oct 9, 2023
11 checks passed
jotak added a commit to jotak/network-observability-operator that referenced this pull request Oct 11, 2023
jotak pushed a commit to jotak/flowlogs-pipeline that referenced this pull request Oct 11, 2023
openshift-ci bot pushed a commit that referenced this pull request Oct 12, 2023
* enhance prom filters

* a|b filter match

Co-authored-by: Julien Pinsonneau <91894519+jpinsonneau@users.noreply.github.com>
jotak added a commit to netobserv/network-observability-operator that referenced this pull request Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm no-doc This PR doesn't require documentation change on the NetObserv operator no-qe This PR doesn't necessitate QE approval 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.

None yet

2 participants