NETOBSERV-2401 Static plugin forms polishing #1028
Conversation
|
Skipping CI for Draft Pull Request. |
jpinsonneau
force-pushed
the
forms_polishing
branch
from
d648d16 to
1dd159e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1028 +/- ##
=======================================
Coverage 54.34% 54.34%
=======================================
Files 203 203
Lines 10792 10792
Branches 1264 1264
=======================================
Hits 5865 5865
Misses 4417 4417
Partials 510 510
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
jpinsonneau
changed the title
jpinsonneau
changed the title
jpinsonneau
force-pushed
the
forms_polishing
branch
from
1dd159e to
8021d94
Compare
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=1a583ab make set-plugin-image |
| <span className="co-pre-line"> | ||
| {t( | ||
| // eslint-disable-next-line max-len | ||
| 'The estimations are based on the number of nodes in the cluster and the sampling rate. They do not take into account the number of namespaces or pods, as their impact is comparatively lower than that of nodes.\nThe estimations are calculated using a linear regression model based on data collected from various OpenShift clusters. Actual resource consumption may vary depending on your specific workload and cluster configuration.\n\nTo change the sampling rate, select a row in the table below.' |
There was a problem hiding this comment.
I think the reason why I was surprised that clicking on the table changed the sampling, is that it was already configured in a previous step.
To make it more explicit, what about doing this:
- No change when the page is loaded first, with "current" written next to the current sampling value
- When the user clicks a row, instead of "current" we would write "previous value" and "new value" next to the corresponding lines
?
There was a problem hiding this comment.
Why not removing it from the previous step and make it clear that you need to pick a sampling in the table ?
I'd like to avoid previous / current value as it feel confusing since the resource is not created yet. We should ask the sampling only once and the resources impact is an important factor for the user to decide which sampling to start with.
If the table is not clear enough, we can add text or even have the sampling dropdown above it.
There was a problem hiding this comment.
Yeah sounds a good idea, to keep sampling configuration only at the end
| <br /> <br /> | ||
| {t( | ||
| // eslint-disable-next-line max-len | ||
| 'This wizard is a helper to create a first FlowCollector resource. It does not cover all the available configuration options, but only the most common ones.\nFor advanced configuration, please use the' |
There was a problem hiding this comment.
| 'This wizard is a helper to create a first FlowCollector resource. It does not cover all the available configuration options, but only the most common ones.\nFor advanced configuration, please use the' | |
| 'This wizard is a helper to create a first FlowCollector resource. It does not cover all the available configuration options, but only the most common ones.\nFor advanced configuration, please use YAML or the' |
There was a problem hiding this comment.
Also can you add something like that, to mention what's in the advanced config:
Advanced configuration includes:
- Filtering options
- Configuring custom exporters
- IP-based custom labels
- Pod identification for secondary networks
- Performance fine-tuning
etc.
You can always edit a FlowCollector later when you start with the simplified configuration.
There was a problem hiding this comment.
-
The YAML been removed in that view so you need to switch form by clicking on the button first.
-
Sure I'm in favor of listing advanced configs here
There was a problem hiding this comment.
On YAML: this is to mention users can still use the non-UI YAML config, I wasn't thinking about the YAML form
| <span className="co-pre-line"> | ||
| {t( | ||
| // eslint-disable-next-line max-len | ||
| 'The example outlined in the table demonstrate a scenario that is tailored to your workload. Consider this example only as a baseline from which adjustments can be made to accommodate your needs.' |
There was a problem hiding this comment.
| 'The example outlined in the table demonstrate a scenario that is tailored to your workload. Consider this example only as a baseline from which adjustments can be made to accommodate your needs.' | |
| 'The example outlined in the table demonstrates a scenario that is tailored to your workload. Consider this example only as a baseline from which adjustments can be made to accommodate your needs.' |
jotak
force-pushed
the
forms_polishing
branch
from
8021d94 to
09cceec
Compare
github-actions
bot
removed
the
ok-to-test
| <span className="co-pre-line"> | ||
| {t( | ||
| // eslint-disable-next-line max-len | ||
| 'Network Observability Operator deploys a monitoring pipeline that consists in:\n - an eBPF agent, that generates network flows from captured packets\n - flowlogs-pipeline, a component that collects, enriches and exports these flows\n - a Console plugin for flows visualization with powerful filtering options, a topology representation and more\n\nFlow data is then available in multiple ways, each optional:\n - As Prometheus metrics\n - As raw flow logs stored in Grafana Loki\n - As raw flow logs exported to a collector\n\nThe FlowCollector resource is used to configure the operator and its managed components.\nThis setup will guide you on the common aspects of the FlowCollector configuration.' |
There was a problem hiding this comment.
FYI: since I was adding more text here (a description of the advanced config), it appeared that this first dialog was bloated of texts ... So I think we can remove the overall description after all, to make it lighter.
Users coming here probably just went through the operator installation which already has its description, so I think it's ok to remove it .. makes sense ?
There was a problem hiding this comment.
Yes, the user is coming from operator installation and must have seen the operator description somehow 👍
| > | ||
| <Button className={`co-pre-line description`} variant="plain" style={{ paddingLeft: 0 }}> | ||
| {`${parts[0]}...`} | ||
| {formatText(parts[0])} {t('(see more...)')} |
There was a problem hiding this comment.
@jpinsonneau on the text ellipsis, I think why I was surprised is that in general ellipsis are used in small constrained space as a necessity. Here there's large space and I didn't expect at first to have to click to read more. I don't know if it's just me... But having a more explicit text would have lessened the "surprise".
| const getSamplings = React.useCallback(() => { | ||
| const current = getCurrentSampling(); | ||
| let samplings = [1, 25, 50, 100, 125, 150]; | ||
| let samplings = [1, 25, 50, 100, 500, 1000]; |
There was a problem hiding this comment.
BTW, I forgot to mention this, I've changed the proposed values here to include higher ones, which we regularly see customers are using
There was a problem hiding this comment.
Is the formula still working fine with those ?
There was a problem hiding this comment.
I'm not sure what "work" means, they continue to provide estimations (with very small consumption on my little cluster obviously), now it's hard to tell if that's accurate
There was a problem hiding this comment.
On my 6-nodes cluster it's over-estimated but the other sampling rates like 50 is also over-estimated, when I inject some test workload
There was a problem hiding this comment.
I think the formula over-estimates the impact of sampling TBH
But we can refine that later, that's still much a TODO
|
/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=daf1abe make set-plugin-image |
jotak
added
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=da852fc make set-plugin-image |
1 similar comment
|
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=da852fc make set-plugin-image |
|
/test plugin-cypress |
|
the default namespace for flowMetric is Not sure if we can read that from flowcollector to have in flowmetric ? at minimum we should clarify in Flowmetric API to use the same namespace and update the default ns to be |
It's not really that the default is
I was thinking either to do that or to make FlowMetrics truly namespace-based, but we need to think more about it before rushing into something IMO... So in the meantime I just added a text to mention which namespace to write here.
I'm surprised that you say that, I think I've added that text already? (not in the API, but in the form) |
|
New changes are detected. LGTM label has been removed. |
github-actions
bot
removed
the
ok-to-test
|
[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 |
|
@memodi as a temporary measure I've set the default to "netobserv", so at least it matches FlowCollector's default, but users still need to make it match if they changed the default. |
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=736ec3a make set-plugin-image |
@jotak - I still see openshift-netobserv-operator as default with newest image: 
I missed the ns recommendation is in the header text I was looking for it under the NS input box |
github-actions
bot
removed
the
ok-to-test
Weird, it works for me ... Are you sure you refreshed the page (in case you had the previous version already loaded)? Do you have specific reproducing steps?
Fair point, I've just added a commit to move that text under Namespace |
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=9995156 make set-plugin-image |
|
@jpinsonneau: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
thanks, it shows up good now. 
|
|
/label qe-approved |
|
/cherry-pick release-1.10 |
|
@jotak: new pull request created: #1051 In 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 kubernetes-sigs/prow repository. |
4 participants
Description
True/Falseto match descriptions and YAML contentDependencies
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.