-
Notifications
You must be signed in to change notification settings - Fork 440
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
Introduce kubernetes package granularity #1018
Conversation
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
inputs: | ||
- type: kubernetes/metrics | ||
title: Collect Kubernetes metrics from Kubelet API | ||
description: Collecting Node, Pod, Container, Volume and System metrics from Kubelet |
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.
Hm, I'm not sure, tbh I have not yet tested this one manually and I don't think I can until elastic/kibana#99866 is merged. @kaiyan-sheng does is it happen you know more about this?
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.
elastic/kibana#99866 is merged but that doesn't include changes to the policy editor. currently the description
field of inputs is not surfaced in the UI (instead we use title
next to the enable/disable input toggle) and I don't foresee the upcoming changes to policy editor to change that. @sorantis if we think it is import to surface description, maybe a quick win would be to add an info icon with a tooltip
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
I gave it a shot. I see that search page looks good: However I see some inconsistencies when I open a “tab” for example kube-state-metrics:
Last but not least data_streams of the input_group selected are not enabled by default. Only data_streams that are defined as “enabled by default” in package are enabled. Not sure if this is proper UX. cc: @sorantis @kaiyan-sheng @jen-huang
|
@ChrsMark looks good! Nit (aligning with the official names https://kubernetes.io/docs/concepts/overview/components/):
What's the difference between Kubelet and Kubernetes tiles? |
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
@sorantis could you elaborate more on this please? Under |
Is it possible to hide either one of them? I recall a conversation where we discussed the possibility to control what tiles we show to the user. cc @jen-huang is it supported? |
@ChrsMark Are you talking about documentation for kubelet, apiserver and etc(for example https://github.com/elastic/integrations/pull/1018/files#diff-3dc8f2b9506ab2ebd53667776cce858fb22dbf740deb62c9687a4da4fc9a1a92)? These are under their own sub tiles once you click the icon of kubelet or apiserver, you can see the documentation there. |
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
With @kaiyan-sheng we figured out the issue related to the docs (reported above). Fixed with a7ea2fc. Long story short we group docs's files by input_group, hence we want 5 docs' files ( |
So what is not addressed yet is to only show data_streams that belong to selected input_group's tile. For example if I select kube-state-metrics I should only see settings for state_* data_streams enabled. @jen-huang @sorantis looking into elastic/kibana#93315 it should be covered by item4 of the list but I'm just raising it here for awareness. If we cover the above shall we completely remove https://github.com/elastic/integrations/blob/master/packages/kubernetes/data_stream/state_container/manifest.yml#L6? |
Yeah, this is not implemented yet as policy editor changes hasn't landed yet (#1018 (comment)). Hold tight :)
IIRC from our discussions we decided to always show the base package tile, plus all the |
we should probably discuss this again, since for integrations with multiple policy_templates (like this Kubernetes example) it can be confusing for the user to understand the difference. |
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
@kaiyan-sheng @sorantis I'm opening this for review since changes in Kibana have landed. I tested again and from my view the UI seems to be as expected. Putting some updated screenshots below: |
I tested the integration manually in a local k8s cluster. Initially added only kube-state-metrics tile and metrics are collected properly. Then I tried to add another tile, kubelet, but I got the following: Is this expected @jen-huang @sorantis ? How I'm supposed to enable more data_streams to the integration? Other than that the package/integration itself looks ok so maybe we can proceed and merge it? What do you think @kaiyan-sheng ? |
@ChrsMark What happens if you change the name of the k8 policy (in step 2)? It sounds like it is autofilling it with a name that was already used by the first k8 policy, can you check if that's the case? If so, that might be a bug. You can also enable more streams by editing the first k8 policy. |
@ChrsMark I think I'm able to add additional data streams in AWS package. For example: I added ec2 metrics first into aws-1 and then added elb metrics into aws-2: These two are added into the policy as two separate inputs. One for collecting EC2 metrics and another one collecting ELB metrics. The policy looks like this:
|
@jen-huang I can verify that adding more integrations under the @jen-huang I think we might need to improve this experience for our users since they will try the straightforward way as I did and they will fail adding extra data_streams. |
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.
Looks good to me!
Thanks @kaiyan-sheng ! @sorantis do you think we can proceed with merging this one? |
@ChrsMark It's a bug if the name of the integration doesn't automatically increment based on how many other k8 integrations on on the selected policy. Actually, we changed some code here recently (reordered Step 1 & 2), so it might be a regression. I'll take a look. |
I just tested this on latest Kibana with system package and found that the name does increment correctly. @ChrsMark do you have any more details around how to reproduce your circumstance? I.e. what is the state of the policy you are adding k8 to (does it already have a k8 integration?), is it the policy that is selected by default in Step 2 or do you have to manually choose it from the dropdown? |
@jen-huang the name does increment correctly for the default policy but if I try to change the policy before adding the integration the name is not adjusted accordingly so this the reason for failing. I think that the name should be recalculated every time you pick a different policy from the dropdown list. Make sense? |
@ChrsMark Ah got it, thanks for that clarification. I think this is an existing behavior that we don't have logged as a bug, so I will open something to track that. |
Thank you @jen-huang! Shall we create a discuss-meta issue to keep track of this one and any other leftovers like keepingVsRemoving the original tile? We can then merge this one since from package perspective works as expected. |
@ChrsMark Is this one going into 7.14? cc @akshay-saraswat |
Yes! |
What does this PR do?
Changes
kubernetes
package to use input groups in order to add icons for each service to support granular search.This PR follows approach used in #767.
This package follows the structure suggested at #1017 (comment).
Checklist
changelog.yml
file.manifest.yml
file to point to the latest Elastic stack release (e.g.^7.13.0
).How to test this PR locally
Related issues
Screenshots