-
Notifications
You must be signed in to change notification settings - Fork 14
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-1113 implements RTT option in Console Plugin #365
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #365 +/- ##
==========================================
- Coverage 57.67% 56.82% -0.86%
==========================================
Files 166 167 +1
Lines 7712 7865 +153
Branches 935 955 +20
==========================================
+ Hits 4448 4469 +21
- Misses 2993 3115 +122
- Partials 271 281 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
70047c4
to
c7ba0a4
Compare
config/sample-frontend-config.yaml
Outdated
@@ -22,9 +22,13 @@ quickFilters: | |||
- name: Services network | |||
filter: | |||
dst_kind: 'Service' | |||
- name: DNS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm how does that work if the feature is off? Is it ignored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's just the sample. The real config comes from operator
https://github.com/netobserv/network-observability-operator/blob/7e7a9f88f1612354d3fa98cbb4d558f2bddc3b74/controllers/consoleplugin/consoleplugin_objects.go#L313-L318
pkg/loki/filter.go
Outdated
intVal, _ := strconv.Atoi(value) | ||
for i := 1; i < len(value); i++ { | ||
nextMin := int((intVal / int(math.Pow10(i))) + 1) | ||
nextMinStr := fmt.Sprintf("%d", nextMin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below: not a big deal as this wouldn't be frequently called, but strconv.Itoa
or strconv.FormatInt
are more efficient to convert ints to strings.
(PS: I've seen this commit is from the DNS PR, but since it's already lgtm you might want to change it here instead)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkg/loki/topology_query.go
Outdated
case "flowRtt": | ||
f = "avg_over_time" | ||
t = "TimeFlowRttNs" | ||
factor = 0.000001 // nanoseconds to miliseconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to avoid any floating point precision issue, it could be better to divide by an int rather than multiply by a decimal
Also, as long as it's only used to inject in a query (as string), that could be written right away as a string (if I'm correct we don't use the numerical value for anything) - that would also avoid that strings.Replace
done below to get the right format for loki :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, a string would be better as it would keep both operator and value at once 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased without extra changes |
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=bb3b18a make set-plugin-image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, will do some tests before approving
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=f6bf6ab make set-plugin-image |
@jpinsonneau I get an error when I'm trying it: This is due to a new input validation check that I added recently: https://github.com/netobserv/network-observability-console-plugin/blob/main/pkg/handler/validation.go#L92-L108 |
Fixed and rebased. Thanks for the check 👍 |
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=5364808 make set-plugin-image |
Thanks Julien
On a more general note, I started testing with the default sampling, and this is really impractical as very few RTTs come up. After switching to sampling 1, it started to look good. I guess that's going to be a known issue, at least for some time, but we should probably document that (not only in the downstream doc, but also in the CRD doc). We can create a new task for that in the 1.5 epic. PS: the issues with the timeseries chart are not blockers I think, if you prefer to keep that for the 1.5 epic; but the Ms/ms should be an easy win, right? |
Sure let's do this: d321f52 We already have the MAX / P90 / P99 issue under the overview enhancement epic: We can put other items under the followup epic: |
We should mention in the UI that these metrics are |
+1, even say "TCP handshakes" as this is limited to that |
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=f37677f make set-plugin-image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Some feedback added to the next epic
@jpinsonneau feel free to implement your suggestion about the chart tooltips mentioning TCP
/approve |
[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 |
This PR will need #361 to manage
FilterComponent.Number
withmore than
capability.Max / P90 / P99 followup:
https://issues.redhat.com/browse/NETOBSERV-1227