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.

Sign up for GitHub

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

Jump to bottom

NETOBSERV-1200 UI: DNSTracking feedback improvments #361

Merged
merged 7 commits into from
Aug 3, 2023
Merged

NETOBSERV-1200 UI: DNSTracking feedback improvments #361

merged 7 commits into from
Aug 3, 2023

Conversation

jpinsonneau
Copy link
Contributor

@jpinsonneau jpinsonneau commented Jul 25, 2023 •
edited
Loading

  • Have console-plugin only accept integers for DNS Id columns.
  • Have filter for DNS Latency column, useful to filter for DNS Latency != 0
    • Allow more than filtering
  • Add DNS latency to overview tab

image
image
image

@codecov
Copy link

codecov bot commented Jul 25, 2023 •
edited
Loading

Codecov Report

Merging #361 (01c1c48) into main (1c39272) will decrease coverage by 0.89%.
Report is 1 commits behind head on main.
The diff coverage is 54.49%.

@@            Coverage Diff             @@
##             main     #361      +/-   ##
==========================================
- Coverage   57.50%   56.61%   -0.89%     
==========================================
  Files         161      164       +3     
  Lines        7276     7614     +338     
  Branches      902      921      +19     
==========================================
+ Hits         4184     4311     +127     
- Misses       2846     3038     +192     
- Partials      246      265      +19
Flag Coverage Δ
uitests 58.02% <54.28%> (-0.35%) ⬇️
unittests 52.58% <54.81%> (-2.23%) ⬇️

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

Files Changed Coverage Δ
pkg/model/fields/fields.go 100.00% <ø> (ø)
web/src/api/loki.ts 100.00% <ø> (ø)
web/src/components/__tests-data__/metrics.ts 100.00% <ø> (ø)
web/src/components/filters/filters-chips.tsx 50.00% <0.00%> (-1.79%) ⬇️
web/src/components/metrics/dropped-donut.tsx 85.18% <ø> (ø)
web/src/components/metrics/histogram.tsx 16.51% <ø> (ø)
...omponents/netflow-topology/2d/topology-content.tsx 28.42% <ø> (ø)
...s/netflow-topology/3d/three-d-topology-content.tsx 8.82% <ø> (ø)
...ponents/netflow-topology/__tests-data__/metrics.ts 100.00% <ø> (ø)
...omponents/netflow-topology/element-panel-stats.tsx 100.00% <ø> (ø)
... and 29 more

... and 1 file with indirect coverage changes

@jpinsonneau jpinsonneau marked this pull request as ready for review July 28, 2023 08:26
@jpinsonneau
Copy link
Contributor Author

Setting this PR as ready for review since it's already quite big
-> max / p90 / p99 can be done in a followup PR

msherif1234
msherif1234 reviewed Jul 31, 2023
View reviewed changes
Copy link
Contributor

@msherif1234 msherif1234 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

/LGTM however the logic moreThanRegex looks too complex but u added UT for it so should be stable we can improve after the PR is merged

@msherif1234
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jul 31, 2023
@jpinsonneau
Copy link
Contributor Author

/LGTM however the logic moreThanRegex looks too complex but u added UT for it so should be stable we can improve after the PR is merged

Yes I agree. If you have an easier approach in mind I'm in 😸

Else @jotak suggested to maintain a static list of value - regex for 1 / 10 / 100 ms. The downside is that it may not apply to any number fields in future.

@jpinsonneau jpinsonneau mentioned this pull request Aug 1, 2023
Merged
5 tasks
@memodi
Copy link
Contributor

memodi commented Aug 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 Aug 1, 2023
@github-actions
Copy link

github-actions bot commented Aug 1, 2023

New image:
quay.io/netobserv/network-observability-console-plugin:40ffaac

It will expire after two weeks.

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

USER=netobserv VERSION=40ffaac make set-plugin-image

memodi
memodi reviewed Aug 1, 2023
View reviewed changes
pkg/loki/filter.go Outdated Show resolved Hide resolved
memodi
memodi requested changes Aug 1, 2023
View reviewed changes
Copy link
Contributor

@memodi memodi left a comment

Choose a reason for hiding this comment

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

moreThan fails to match

@openshift-ci openshift-ci bot removed the lgtm label Aug 1, 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 Aug 2, 2023
@jpinsonneau
Copy link
Contributor Author

moreThan fails to match

Good catch @memodi, sorry about that. I tried to manage more cases but ended breaking it 👼
I removed the start match as you suggested 01c1c48 since it's managed by the prefix part DnsLatencyMs\":
Thanks !

@jpinsonneau jpinsonneau added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Aug 2, 2023
@github-actions
Copy link

github-actions bot commented Aug 2, 2023

New image:
quay.io/netobserv/network-observability-console-plugin:9407a24

It will expire after two weeks.

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

USER=netobserv VERSION=9407a24 make set-plugin-image

@memodi
Copy link
Contributor

memodi commented Aug 2, 2023

/label qe-approved
thanks @jpinsonneau for incorporating feedback.

@openshift-ci openshift-ci bot added the qe-approved QE has approved this pull request label Aug 2, 2023
@msherif1234
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Aug 2, 2023
@jpinsonneau
Copy link
Contributor Author

/approve

@openshift-ci
Copy link

openshift-ci bot commented Aug 3, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpinsonneau

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 Aug 3, 2023
@openshift-merge-robot openshift-merge-robot merged commit 13e24e4 into netobserv:main Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msherif1234 msherif1234 msherif1234 left review comments

@memodi memodi memodi requested changes

@jotak jotak Awaiting requested review from jotak

@OlivierCazade OlivierCazade Awaiting requested review from OlivierCazade

Assignees

@msherif1234 msherif1234

@memodi memodi

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jpinsonneau @msherif1234 @memodi @openshift-merge-robot