Skip to content
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

Merged
merged 10 commits into from
Jun 28, 2021

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented May 24, 2021

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

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • If I'm introducing a new feature, I have modified the Kibana version constraint in my package's manifest.yml file to point to the latest Elastic stack release (e.g. ^7.13.0).

How to test this PR locally

  1. Search for the integration in integrations search in Kibana
  2. Check that all tiles are present
  3. Select each tile and verify content is as expected.

Related issues

Screenshots

Screenshot 2021-05-27 at 3 38 55 PM

Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark self-assigned this May 24, 2021
@elasticmachine
Copy link

elasticmachine commented May 24, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Branch indexing

  • Start Time: 2021-06-24T22:28:34.753+0000

  • Duration: 27 min 30 sec

  • Commit: 07b9cd2

Test stats 🧪

Test Results
Failed 0
Passed 109
Skipped 0
Total 109

Trends 🧪

Image of Build Times

Image of Tests

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

Choose a reason for hiding this comment

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

@ChrsMark @mtojek folks, is input description visible anywhere in the UI?

Copy link
Member Author

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?

Copy link
Contributor

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>
@ChrsMark
Copy link
Member Author

ChrsMark commented May 27, 2021

I gave it a shot. I see that search page looks good:
Screenshot 2021-05-27 at 3 38 55 PM

However I see some inconsistencies when I open a “tab” for example kube-state-metrics:

  1. Docs for metricsets/data_streams are missing and I only see the basic docs
    Screenshot 2021-05-27 at 3 46 47 PM
  2. The description of the integration in settings view is the one of kubelet input_group
    Screenshot 2021-05-27 at 3 46 23 PM

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

Note: I have not managed to manually test the integration itself to check if metrics are being collected since I'm still trying to enrol an agent running on k8s so as to test it properly.
Using elastic-package tool I'm able to run test for k8s and have an enrolled Agent and hence I'm able to later manually add integration for that Agent too. It seems to collect metrics from Kubelet's stats API.

@sorantis
Copy link

@ChrsMark looks good! Nit (aligning with the official names https://kubernetes.io/docs/concepts/overview/components/):

  • Kube-state-metrics -> kube-state-metrics
  • Api-server -> kube-apiserver
  • Proxy -> kube-proxy
  • Controller-manager -> kube-controller-manager
  • Scheduler -> kube-scheduler
  • Event -> Events

What's the difference between Kubelet and Kubernetes tiles?

Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
@ChrsMark
Copy link
Member Author

ChrsMark commented May 27, 2021

Screenshot 2021-05-27 at 5 42 13 PM
Updated the naming ^^^

What's the difference between Kubelet and Kubernetes tiles?

@sorantis could you elaborate more on this please? Under policy_templates we have 7 input_groups but UI apparently adds one extra tile (Kubernetes) for the whole integration I guess?

@sorantis
Copy link

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?

@kaiyan-sheng
Copy link
Contributor

  1. Docs for metricsets/data_streams are missing and I only see the basic docs

@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>
@ChrsMark
Copy link
Member Author

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 (.md) one per input_group. Various data_streams' docs are group under its respective input_group's docs file.

@ChrsMark
Copy link
Member Author

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?

@jen-huang
Copy link
Contributor

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.

Yeah, this is not implemented yet as policy editor changes hasn't landed yet (#1018 (comment)). Hold tight :)

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?

IIRC from our discussions we decided to always show the base package tile, plus all the policy_template tiles.

@sorantis
Copy link

sorantis commented Jun 2, 2021

IIRC from our discussions we decided to always show the base package tile, plus all the policy_template tiles.

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>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark marked this pull request as ready for review June 16, 2021 14:07
@ChrsMark
Copy link
Member Author

@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:

Screenshot 2021-06-16 at 5 01 03 PM

Screenshot 2021-06-16 at 5 01 17 PM

Screenshot 2021-06-16 at 5 01 36 PM

Screenshot 2021-06-16 at 5 02 18 PM

@ChrsMark
Copy link
Member Author

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:

Screenshot 2021-06-17 at 4 16 12 PM

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 ?

@jen-huang
Copy link
Contributor

@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.

@kaiyan-sheng
Copy link
Contributor

kaiyan-sheng commented Jun 17, 2021

@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:
Screen Shot 2021-06-17 at 2 51 49 PM

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:

id: f5ce3420-cf7f-11eb-a518-f7d4117bebd4
revision: 12
outputs:
  default:
    type: elasticsearch
    hosts:
      - 'http://localhost:9200'
output_permissions:
  default:
    aws-1:
      indices:
        - names:
            - metrics-aws.ec2_metrics-default
          privileges:
            - auto_configure
            - create_doc
    aws-2:
      indices:
        - names:
            - metrics-aws.elb_metrics-default
          privileges:
            - auto_configure
            - create_doc
    _elastic_agent_checks:
      cluster:
        - monitor
      indices:
        - names:
            - logs-elastic_agent.*-default
            - metrics-elastic_agent.*-default
          privileges:
            - auto_configure
            - create_doc
agent:
  monitoring:
    enabled: true
    use_output: default
    namespace: default
    logs: true
    metrics: true
inputs:
  - id: b46ec663-1743-47f1-b8d8-03c90a0c38e1
    name: aws-1
    revision: 1
    type: aws/metrics
    use_output: default
    meta:
      package:
        name: aws
        version: 0.6.2
    data_stream:
      namespace: default
    streams:
      - id: aws/metrics-aws.ec2_metrics-b46ec663-1743-47f1-b8d8-03c90a0c38e1
        data_stream:
          dataset: aws.ec2_metrics
          type: metrics
        metricsets:
          - ec2
        period: 5m
        credential_profile_name: elastic-beats
        tags_filter: null
  - id: d44a360d-dc64-4f89-a113-d7bba4d15f14
    name: aws-2
    revision: 1
    type: aws/metrics
    use_output: default
    meta:
      package:
        name: aws
        version: 0.6.2
    data_stream:
      namespace: default
    streams:
      - id: aws/metrics-aws.elb_metrics-d44a360d-dc64-4f89-a113-d7bba4d15f14
        data_stream:
          dataset: aws.elb_metrics
          type: metrics
        metricsets:
          - elb
        period: 1m
        credential_profile_name: elastic-beats
        tags_filter: null
fleet:
  hosts:
    - 'http://localhost:8220'

@ChrsMark
Copy link
Member Author

@jen-huang I can verify that adding more integrations under the k8s policy works fine. Also when I'm trying from integrations page to add extra data_streams then I have to change the name of the integration, so this seems to be the issue:

Screenshot 2021-06-18 at 10 26 59 AM

@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.

Copy link
Contributor

@kaiyan-sheng kaiyan-sheng left a 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!

@ChrsMark
Copy link
Member Author

Thanks @kaiyan-sheng !

@sorantis do you think we can proceed with merging this one?

@jen-huang
Copy link
Contributor

when I'm trying from integrations page to add extra data_streams then I have to change the name of the integration

@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.

@jen-huang
Copy link
Contributor

when I'm trying from integrations page to add extra data_streams then I have to change the name of the integration

@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?

@ChrsMark
Copy link
Member Author

@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?

@jen-huang
Copy link
Contributor

@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.

@ChrsMark
Copy link
Member Author

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.

@sorantis sorantis self-requested a review June 24, 2021 11:24
@ChrsMark ChrsMark merged commit 6891bb8 into elastic:master Jun 28, 2021
james-elastic pushed a commit to james-elastic/integrations that referenced this pull request Jun 30, 2021
@sorantis
Copy link

@ChrsMark Is this one going into 7.14? cc @akshay-saraswat

@ChrsMark
Copy link
Member Author

Yes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add granularity on Kubernetes integration
5 participants