Skip to content

Conversation

jotak
Copy link
Member

@jotak jotak commented Aug 13, 2024

  • 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

Description

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.

  • Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
  • Does this PR require product documentation?
    • If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
  • Does this PR require a product release notes entry?
    • If so, fill in "Release Note Text" in the JIRA.
  • Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
    • If so, make sure it is described in the JIRA ticket.
  • QE requirements (check 1 from the list):
    • Standard QE validation, with pre-merge tests unless stated otherwise.
    • Regression tests only (e.g. refactoring with no user-facing change).
    • No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Aug 13, 2024

@jotak: This pull request references NETOBSERV-1798 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.18.0" version, but no target version was set.

In response to this:

  • 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

Description

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.

  • Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
  • Does this PR require product documentation?
  • If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
  • Does this PR require a product release notes entry?
  • If so, fill in "Release Note Text" in the JIRA.
  • Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
  • If so, make sure it is described in the JIRA ticket.
  • QE requirements (check 1 from the list):
  • Standard QE validation, with pre-merge tests unless stated otherwise.
  • Regression tests only (e.g. refactoring with no user-facing change).
  • No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

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.

Copy link

openshift-ci bot commented Aug 13, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jotak. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jotak
Copy link
Member Author

jotak commented Aug 13, 2024

@jpinsonneau FYI I need to check how that fits with #571 - some bits need probably to be adapted

Copy link

codecov bot commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 13.88889% with 31 lines in your changes missing coverage. Please review.

Project coverage is 56.21%. Comparing base (aef2627) to head (5a34fc9).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pkg/handler/resources.go 16.66% 14 Missing and 1 partial ⚠️
pkg/prometheus/client.go 0.00% 12 Missing ⚠️
web/src/model/filters.ts 33.33% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #577      +/-   ##
==========================================
- Coverage   56.29%   56.21%   -0.08%     
==========================================
  Files         190      190              
  Lines        9289     9303      +14     
  Branches     1197     1197              
==========================================
+ Hits         5229     5230       +1     
- Misses       3691     3704      +13     
  Partials      369      369              
Flag Coverage Δ
uitests 57.79% <33.33%> (-0.03%) ⬇️
unittests 51.87% <10.00%> (-0.20%) ⬇️

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

Files with missing lines Coverage Δ
...components/toolbar/filters/autocomplete-filter.tsx 63.33% <ø> (-0.80%) ⬇️
web/src/model/filters.ts 73.33% <33.33%> (-2.11%) ⬇️
pkg/prometheus/client.go 0.00% <0.00%> (ø)
pkg/handler/resources.go 15.54% <16.66%> (+0.32%) ⬆️

@jotak
Copy link
Member Author

jotak commented Aug 13, 2024

@jpinsonneau FYI I need to check how that fits with #571 - some bits need probably to be adapted

Actually, forget about it: I was thinking about removing the workaround that says

// This is because multi-tenancy is currently not managed for prom datasource, hence such queries have to go with Loki [etc.]

since now we handle multi-tenancy via prom's in the dev console; but that workaround is still useful when non-admin users run queries from the admin console, ie. not namespace-constrained.

So I think this PR is ok (there will just be minor conflicts with the other one)

Comment on lines +121 to +136
if code == http.StatusUnauthorized || code == http.StatusForbidden {
// In case this was a prometheus 401 / 403 error, the query is repeated with Loki
// This is because multi-tenancy is currently not managed for prom datasource, hence such queries have to go with Loki
// Unfortunately we don't know a safe and generic way to pre-flight check if the user will be authorized
hlog.Info("Retrying with Loki...")
// continuing with loki below
} else {
return nil, code, fmt.Errorf("error while fetching label %s values from Prometheus: %w", label, err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we'll need that case as we get https://github.com/netobserv/network-observability-console-plugin/pull/589/files#diff-017f9ad356145a3e9c8d08ec33d1b5e694bc4820f301ad6d5c6470005eaf8a38 changes 🤔

The namespace was missing in the query params so we always query the admin prom

Copy link
Member Author

@jotak jotak Sep 9, 2024

Choose a reason for hiding this comment

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

the error is not only in dev perspective but also in admin perspective, when logged in with a non-cluster-admin.. so I think it's still relevant for that case

Copy link
Contributor

Choose a reason for hiding this comment

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

correct, let's keep it then !

- 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
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented Sep 10, 2024

@jotak: This pull request references NETOBSERV-1798 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.18.0" version, but no target version was set.

In response to this:

  • 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

Description

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.

  • Is this PR backed with a JIRA ticket? If so, make sure it is written as a title prefix (in general, PRs affecting the NetObserv/Network Observability product should be backed with a JIRA ticket - especially if they bring user facing changes).
  • Does this PR require product documentation?
  • If so, make sure the JIRA epic is labelled with "documentation" and provides a description relevant for doc writers, such as use cases or scenarios. Any required step to activate or configure the feature should be documented there, such as new CRD knobs.
  • Does this PR require a product release notes entry?
  • If so, fill in "Release Note Text" in the JIRA.
  • Is there anything else the QE team should know before testing? E.g: configuration changes, environment setup, etc.
  • If so, make sure it is described in the JIRA ticket.
  • QE requirements (check 1 from the list):
  • Standard QE validation, with pre-merge tests unless stated otherwise.
  • Regression tests only (e.g. refactoring with no user-facing change).
  • No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

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.

@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

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 kubernetes-sigs/prow repository.

@jotak
Copy link
Member Author

jotak commented Sep 16, 2024

closing because I'm merging this PR into Julien's #589

@jotak jotak closed this Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants