NETOBSERV-2477: scope warning banner#1262
NETOBSERV-2477: scope warning banner#1262jpinsonneau wants to merge 2 commits intonetobserv:mainfrom
Conversation
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@jotak should we add |
|
/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=410f991 make set-plugin-image |
|
/label qe-approved |
jotak
changed the title
|
@jpinsonneau: This pull request references NETOBSERV-2477 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
| if (isPromMissingLabelError(errStr) && promErrStr !== model.chipsPopoverMessage) { | ||
| // check if it's a prom missing label error | ||
| if (isPromMissingLabelError(errStr)) { | ||
| // First, check if scope needs to be changed (topology only) |
There was a problem hiding this comment.
why is it topology only? Won't we have the same issue on Overview?
There was a problem hiding this comment.
Let me see what I can do here. Overview is displaying per graphs errors and the scope slider is only available when no graph are focussed so it's a bit more complex but probably interesting in that case.
There was a problem hiding this comment.
BTW, adding the missing metrics in the operator will result in having isPromDisabledMetricsError instead of isPromMissingLabelError.
So in the end, do we want to automatically switch scope when the metric is disabled ?
Maybe it would be better to hide those ? 🤔
There was a problem hiding this comment.
Here is an attempt but the questions above remains
a7895c2
| // check if it's a prom missing label error and remove filters | ||
| // when the prom error is different to the new one | ||
| if (isPromMissingLabelError(errStr) && promErrStr !== model.chipsPopoverMessage) { | ||
| // check if it's a prom missing label error |
There was a problem hiding this comment.
let me check if I understand correctly: there were 2 problems here, the "broken scope" issue due to missing labels, and the filters not being disabled as they should due to a bad combination of scopes+filters, which iiuc is really what is described in the ticket.
For the latter, what was the bug? Is it this promErrStr !== model.chipsPopoverMessage check, that you removed? (trying to understand why you removed that)
There was a problem hiding this comment.
Filters are disabled when involved. That's still working.
The scope never been handled and the fact that the error page is showing fullscreen makes it hard to the user to figure out he needs to change scope in the advanced options (scope slider was hidden in that case).
The "simple" scenario is when scope needs missing labels; we find a compatible scope and the query reloads properly.
The "complex" case is when both are involved:
- scope will fallback to a compatible scope and query again
- then the query will fail because of the filters not working on the new scope
- finally the query will run with new scope and filters disabled
Instead of checking chipsPopoverMessage for filters and scopeWarning for scopes, it's easier to trigger those only when needed:
isPromMissingLabelError(errStr) as soon as we have a prom label missing
- fallbackScope && fallbackScope.id !== currentScope.id for scope
- !fv.disabled for filters
If none of those are catching the error, we still show the full error message
yes, maybe just on |
|
github-actions
bot
removed
the
ok-to-test
|
/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=4961546 make set-plugin-image |
|
@jpinsonneau: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Description
Some topology scopes may not be available in prometheus. This result in a trap where user can't change scope directly.
This handle automatically switching scope putting a warning on top of the view as same as the disable filters mechanism already existing.
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.