-
Notifications
You must be signed in to change notification settings - Fork 26
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
Dependency Plugin Additional Options #200
Conversation
Codecov Report
@@ Coverage Diff @@
## main #200 +/- ##
==========================================
+ Coverage 69.76% 70.12% +0.36%
==========================================
Files 32 34 +2
Lines 3545 3776 +231
Branches 0 43 +43
==========================================
+ Hits 2473 2648 +175
- Misses 924 973 +49
- Partials 148 155 +7
|
build/charts/theia/README.md
Outdated
@@ -52,7 +52,7 @@ Kubernetes: `>= 1.16.0-0` | |||
| grafana.enable | bool | `true` | Determine whether to install Grafana. It is used as a data visualization and monitoring tool. | | |||
| grafana.homeDashboard | string | `"homepage.json"` | Default home dashboard. | | |||
| grafana.image | object | `{"pullPolicy":"IfNotPresent","repository":"projects.registry.vmware.com/antrea/theia-grafana","tag":"8.3.3"}` | Container image used by Grafana. | | |||
| grafana.installPlugins | list | `["https://downloads.antrea.io/artifacts/grafana-custom-plugins/theia-grafana-sankey-plugin-1.0.2.zip;theia-grafana-sankey-plugin","https://downloads.antrea.io/artifacts/grafana-custom-plugins/theia-grafana-chord-plugin-1.0.1.zip;theia-grafana-chord-plugin","https://downloads.antrea.io/artifacts/grafana-custom-plugins/theia-grafana-dependency-plugin-1.0.0.zip;theia-grafana-dependency-plugin","grafana-clickhouse-datasource 1.0.1"]` | Grafana plugins to install. | | |||
| grafana.installPlugins | list | `["https://downloads.antrea.io/artifacts/grafana-custom-plugins/theia-grafana-sankey-plugin-1.0.2.zip;theia-grafana-sankey-plugin","https://downloads.antrea.io/artifacts/grafana-custom-plugins/theia-grafana-chord-plugin-1.0.1.zip;theia-grafana-chord-plugin","https://github.com/Dhruv-J/grafana-dependency-plugin/archive/refs/tags/pre3.zip;theia-grafana-dependency-plugin","grafana-clickhouse-datasource 1.0.1"]` | Grafana plugins to install. | |
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.
Just curious, why we change the path here?
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 link I use for the test release, so if anyone wants to run my PR branch, they can without any additional setup. I change it back before the final merge occurs.
970a510
to
40bcf48
Compare
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.
Would you mind adding either more explanation in the PR description or your feature design in the related issue? Including example screenshots of the new feature, for both the pod label toggle and the color choice.
It is not very clear to me
- how does they look like in Grafana
- how we expect the pod label toggle to work? why we only look for "k8s"/"k8s-app" labels?
I thought we had the discussion before on the previous PR: #142 (comment)
} | ||
console.log('attempting to log JSON: '+labelJSON); | ||
let labels = JSON.parse(labelJSON); | ||
if(labels['app'] !== undefined) { |
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.
little confused, why we only look for "app" and "k8s-app" labels?
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.
In order to group by labels, I added support for the two basic ones I saw that had values for most pods. Should I make it check against a user-defined label?
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.
the two basic ones I saw that had values for most pods
to clarify, where does the observation come from? which are the "most pods"? like I mentioned in the previous comment, is "app.kubernetes.io/name" less common than those two per your observation?
My point is, I'm not sure about which particular labels should we look for, so I doubt whether it's a good way to design this feature. I would like to take one step back to the motivation and design. For example, I see there are some common labels listed in k8s documentation: https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/ , but I'm not convinced that we should look for these labels.
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.
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.
- Could you also upload the screenshot of the diagram?
- Does it mean user can only specify one label each time? Could we make use of the ad-hoc filter, showing all the available labels, and letting users to select one of more labels they would like to group by? I think a drop-down list is better than a text box, as
a. users may not know which labels they have right now in the cluster, if we don't give them the options
b. they can select multiple labels at once.
Although I'm not sure if it is doable. If you would like to do a design review/have a design doc, we can discuss it more in details.
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.
I'll update with a screenshot of the diagram as well, but I don't think that an ad-hoc filter would be feasible for a couple reasons.
- If pods are grouped by label, their pod label is used per square instead of the pod name itself. If multiple pod labels were used at once, I don't think there would be a good way of grouping them, especially if there are conflicts among labels. For example two pods could both be labelled by the same value for one label, but different values for another.
- Ad-hoc filter can only check the database columns, anything more specific would require query variables, which cannot be included in the query if the query is to pass end to end testing. Dropdown menu is a great idea, I don't think it could be populated without a query variable. The options data structure that's passed to the Panel contains the editing options for the panel itself. That means that the options for the panel can't access the data the panel accesses in order to populate its members.
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.
For the first one, I don't have a strong opinion on whether we should enable single label or multiple labels selection. If single label selection would make the graph representation easier, we can go with it.
For example two pods could both be labelled by the same value for one label, but different values for another.
Although I didn't get how does the above situation harm.
For the second one, we can remove this query from E2E test, if necessary. I think it worth exploring on query variables, ad-hoc filters, and maybe other Grafana configurations to see if it will make the dropdown menu work. I concern more on whether we can parse the Pod label string and split them into individual key value pairs.
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.
Yeah, splitting the label is more challenging than getting the rest of the query variable to work, since distinct values of only the key needs to be selected. There isn't a query that can parse the JSON and format output, so it may not be possible to include a dropdown menu. I hope that users wishing to group by label will already have some idea for which labels they wish to group by.
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.
Could you please also update network-flow-visibility.md to add the introduction of the new options?
### 3. Customize the Panel Options | ||
|
||
Users can customize the panel by choosing to group the diagram based a chosen | ||
label. It is also possible to change the color of the pod squares in the |
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.
label -> Pod labels
pod -> Pod
return sourcePodName; | ||
} | ||
|
||
// TODO add button or switch to set groupByLabelVariable |
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.
little confused on the TODO comment here. What is it for? I thought we already have a switch in the panel options?
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.
I meant to delete that earlier
export const plugin = new PanelPlugin<DependencyOptions>(DependencyPanel).setPanelOptions((builder) => builder); | ||
export const plugin = new PanelPlugin<DependencyOptions>(DependencyPanel).setPanelOptions((builder) => builder.addBooleanSwitch({ | ||
path: 'groupByLabel', | ||
name: 'Group By Label', |
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.
I think it would be better to change to Group by Pod Label
@@ -1,5 +1,9 @@ | |||
# Changelog | |||
|
|||
## 1.0.2 - 04-xx-2023 | |||
|
|||
Added label view toggle and color choice. |
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.
Added "Group by Pod Labels" toggle and Pod square color choices.
I left it off because it was part of the dependency plugin instead, so I added it to the README for that. I can also add it to flow-visibility.md if it would fit there. |
Eventually it is a user option, right? Normally Theia users would only read network-flow-visibility.md but not plugin README. I think it would be good to let users know they have these options. Plugin README is more like a documentation for developers who would like to use or build on top of the plugin. |
docs/network-flow-visibility.md
Outdated
@@ -764,6 +764,12 @@ the selected time range. | |||
|
|||
<img src="https://downloads.antrea.io/static/04132023/flow-visibility-network-topology-0.png" width="400" alt="Network Topology Dashboard service dependency graph"> | |||
|
|||
Users can group Pods by label, allowing Pod squares in the diagram to represent | |||
a set of pods with the same label value. It is also possible to choose the |
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.
a set of pods with the same label value. It is also possible to choose the | |
a set of Pods with the same label value. It is also possible to choose the |
docs/network-flow-visibility.md
Outdated
@@ -764,6 +764,12 @@ the selected time range. | |||
|
|||
<img src="https://downloads.antrea.io/static/04132023/flow-visibility-network-topology-0.png" width="400" alt="Network Topology Dashboard service dependency graph"> | |||
|
|||
Users can group Pods by label, allowing Pod squares in the diagram to represent |
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.
maybe add some instructions to let users know where should they find the configurations, telling users it is in panel edit
build/charts/theia/README.md
Outdated
@@ -52,7 +52,7 @@ Kubernetes: `>= 1.16.0-0` | |||
| grafana.enable | bool | `true` | Determine whether to install Grafana. It is used as a data visualization and monitoring tool. | | |||
| grafana.homeDashboard | string | `"homepage.json"` | Default home dashboard. | | |||
| grafana.image | object | `{"pullPolicy":"IfNotPresent","repository":"projects.registry.vmware.com/antrea/theia-grafana","tag":"8.3.3"}` | Container image used by Grafana. | | |||
| grafana.installPlugins | list | `["https://downloads.antrea.io/artifacts/grafana-custom-plugins/theia-grafana-sankey-plugin-1.0.2.zip;theia-grafana-sankey-plugin","https://downloads.antrea.io/artifacts/grafana-custom-plugins/theia-grafana-chord-plugin-1.0.1.zip;theia-grafana-chord-plugin","https://downloads.antrea.io/artifacts/grafana-custom-plugins/theia-grafana-dependency-plugin-1.0.1.zip;theia-grafana-dependency-plugin","grafana-clickhouse-datasource 1.0.1"]` | Grafana plugins to install. | | |||
| grafana.installPlugins | list | `["https://downloads.antrea.io/artifacts/grafana-custom-plugins/theia-grafana-sankey-plugin-1.0.2.zip;theia-grafana-sankey-plugin","https://downloads.antrea.io/artifacts/grafana-custom-plugins/theia-grafana-chord-plugin-1.0.1.zip;theia-grafana-chord-plugin","https://downloads.antrea.io/artifacts/grafana-custom-plugins/theia-grafana-chord-plugin-1.0.2.zip;theia-grafana-dependency-plugin","grafana-clickhouse-datasource 1.0.1"]` | Grafana plugins to install. | |
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.
chord -> dependency
- field 5: destinationServicePortName value with name or an alias of `destinationServicePortName` | ||
- field 6: octetDeltaCount value with name or an alias of `octetDeltaCount` | ||
- field 2: sourcePodLabels value with name or alias of `sourcePodLabels` | ||
- field 3: sourcePodNamespace value with name or alias of `sourcePodNamespace` |
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.
why we add sourcePodNamespace
here? Is it being used anywhere in the source code?
This PR allows for users to edit the panel and choose to group pods by labels or not. The user can also change the color of the boxes to red, yellow, green, or blue. Fixes issue antrea-io#164 Signed-off-by: Dhruv-J <dhruvj@vmware.com>
/theia-test-e2e |
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
This PR allows for users to edit the panel and choose to
group pods by labels or not. The user can also change the
color of the boxes to red, yellow, green, or blue.
Fixes issue #164