NETOBSERV-1538 & NETOBSERV-1819 & NETOBSERV-1813 & NETOBSERV-1812 & NETOBSERV-1798 error messages and UI polishing #589
Conversation
|
Skipping CI for Draft Pull Request. |
jpinsonneau
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #589 +/- ##
==========================================
- Coverage 56.27% 56.03% -0.25%
==========================================
Files 190 193 +3
Lines 9304 9457 +153
Branches 1201 1207 +6
==========================================
+ Hits 5236 5299 +63
- Misses 3698 3784 +86
- Partials 370 374 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
jpinsonneau
force-pushed
the
1538
branch
2 times, most recently
from
bfe940b to
cc96079
Compare
jpinsonneau
force-pushed
the
1538
branch
2 times, most recently
from
c57f769 to
e25c6cd
Compare
jpinsonneau
changed the title
jpinsonneau
added
the
ok-to-test
|
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=7b4256d make set-plugin-image |
|
|
||
| getStatus(ContextSingleton.getForcedNamespace()) | ||
| .then(status => { | ||
| console.info('status result', status); |
There was a problem hiding this comment.
I feel it's useful to keep that in the console in case the UI don't show everything but I can remove it if you prefer
pkg/handler/status.go
Outdated
| IsAllowLoki bool `yaml:"isAllowLoki" json:"isAllowLoki"` | ||
| LokiNamespacesCount int `yaml:"lokiNamespacesCount" json:"lokiNamespacesCount"` | ||
| IsLokiReady bool `yaml:"isLokiReady" json:"isLokiReady"` | ||
| IsConsistent bool `yaml:"isConsistent" json:"isConsistent"` |
There was a problem hiding this comment.
If I'm right, you can remove this IsConsistent from the REST API because it's not used in the js code - it's re-computed there (and I'd be in favor to keep it that way, computed on frontend side only, since it has all the data to compute it).
Also, we need to see if we find false-positives of this consistency check - I'm fine to keep it for the time being, but given that metrics are configurable, I'm wondering if there could be undesired "inconsistent" triggers. We'll see
| status.isAllowProm && | ||
| status.lokiNamespacesCount !== status.promNamespacesCount && ( | ||
| <Text component={TextVariants.blockquote}> | ||
| {t(`Loki and Prom storages are not consistent. Check health dashboard for errors.`)} |
There was a problem hiding this comment.
Prom => Prometheus. Also, I'd add more details to this text, because "not consistent" isn't very clear. Maybe "Loki and Prometheus storages seem diverging, they do not capture the same number of namespaces" ?
Also, a potential cause of trigger is when new namespaces are created, flows being caught in loki faster than in prometheus, so I guess this message will display in such case. That could make the users worry for nothing?
There was a problem hiding this comment.
Actually, this is only showed when an error occurred right? When the backend returns data, it won't be displayed? If so, then I guess it's not that an issue to have a small amount of false positives
There was a problem hiding this comment.
This is only showing when an error or empty content is displayed indeed
| return props.resetDefaultFilters; | ||
| } | ||
| return undefined; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
why disable linter, it seems to have already all the needed deps?
There was a problem hiding this comment.
Without that, the linter expect props to be the dependency here and it will refresh on every change.
There was a problem hiding this comment.
we can destructure props (just the bits we need) to get them back: b42aaca
| return props.clearFilters; | ||
| } | ||
| return undefined; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
|
Hey @jpinsonneau , |
pkg/handler/status.go
Outdated
| // get namespaces using Prom | ||
| promNamespaces, code, err := h.getNamespacesValues(ctx, promClients, isDev) | ||
| if err != nil { | ||
| writeError(w, code, "Error while fetching label namespace values from Prometheus: "+err.Error()) |
There was a problem hiding this comment.
Here and below: the downside of throwing an error is that on the frontend side we loose the information about what is enabled (loki and/or prom). Since this is a status check, what about, instead of throwing an error, just set false to IsLoki/PromReady + add the error as a message embedded in the Status result?
There was a problem hiding this comment.
To illustrate:
Here on the frontend side we could determine what to display based on if it's a loki or a prometheus permission error, but we don't know that due to the status endpoint failing to provide the status.
github-actions
bot
removed
the
ok-to-test
I have reduced the scope slider height following this ticket https://issues.redhat.com/browse/NETOBSERV-1813 |
Co-authored-by: Joel Takvorian <[email protected]>
- When there's permission issue with prom user, fallback on using loki (like we also do for regular metrics queries) - Do not try using prom for labels not in prom metrics - Fix misleading error messages - Do not disallow filter creation in case of autocompletion error
jotak
changed the title
jotak
added
the
ok-to-test
|
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=3cf9850 make set-plugin-image |
I would say that hiding it completely is worse than having some text overlap? It's still useful despite the cosmetic issue... |
Yeah that would be better indeed. This is a temporary fix until we do https://issues.redhat.com/browse/NETOBSERV-1476 |
|
text added for prometheus errors / troubleshooting: |
github-actions
bot
removed
the
ok-to-test
|
/lgtm |
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Added a small change 55d93e0 LGTM |
|
/ok-to-test |
openshift-ci
bot
added
the
ok-to-test
|
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=e61421c make set-plugin-image |
|
Merging this for branch cut. Please create followup bug if needed |
|
/label qe-approved |
3 participants
Description
- improved /status endpoint to return more info
- get namespaces count per storage to ensure consistency
- added link to FlowCollector CR
Also took the opportunity to fix the following issues as it was small fixes:
- reduced minimum display height
- forced CSS color
As this PR is getting bigger and bigger, I have created followups: https://issues.redhat.com/browse/NETOBSERV-1828 & https://issues.redhat.com/browse/NETOBSERV-1829
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.