Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature/istio metrics #4253
Feature/istio metrics #4253
Changes from all commits
8db832b
c2ac3cd
16169bf
f8b1771
91085a8
3986911
1c236fa
2a6097a
61f1e34
9d1bbb2
cd34265
b9daa35
e377a92
c1ed41d
45a9664
e899bb3
6411126
64a2987
a2dc801
8cb83b9
d7770ee
c604e81
5e79ed2
8d20cc0
3184b75
12adee5
7f8b11d
16a052c
e0cc33e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can u add a section on how to test it locally? Steps how to install istio, config until you see metrics in ELK?
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 don't think all the instructions on how to test this locally should end up in the official docs. It doesn't seem to be standard practise in other packages. In order to test it "locally" you need to setup all sort of VMs, K8s cluster and elastic stack with custom properties. I don't think this kind of information should end up in this readme. For the istio specific steps to set this up, I am using the getting started at https://istio.io/latest/docs/setup/getting-started/ which is already documented in the issue.
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.
We tried to change this a little: have a look here https://github.com/elastic/integrations/tree/main/packages/nginx_ingress_controller/_dev/build#how-to-setup-and-test-ingress-controller-locally
In general of course not instructions to install all components, you will consider that user has already elk and k8s etc. But reference to starting guide with some additional hints on what to install would be great I think
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.
Ok. I can add something similar but fortunately there is not much custom configs or manual steps that I needed for either logs or metrics
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 there are other packages that have additionla readme files in testing that describe in detail how it should be tested. So users using the package will not see it but any package dev has easy access to it.
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.
@ruflin reading your comment I would suggest that a more appropriate place for testing docs is packages/istio/data_stream/istiod_metrics/_dev/test/system or anywhere else hidden in the test folders. If I understand correctly adding this test docs here (at packages/istio/_dev/build/docs/README.md will modify the packages/istio/docs/README.md which is used to expose the package docs to the package users).
I'm not a fan of mixing testing docs with official user facing docs where we usually document only the fields exposed by the integration.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
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.
Doing a rehearsal from https://www.elastic.co/blog/istio-monitoring-with-elastic-observability I remember that
istiod
could be accessed directly through its service. For example:Metricbeat config:
So in that case you don't need a to automatically discover the Pods using a condition, since you can directly access the endpoint through the k8s Service:
istiod.istio-system:15014
whereistiod
is the name of the Service andistio-system
is the name of Namespace it belongs to.Is this still the case or there is sth that has been changed?
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.
Also any update on this @gsantoro ? The Pr can be merged for now and open a small enhancement for this if you think you need time
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 I prefer to merge and then open a new issue for that.
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.
But is
istiod
exposed through a Service today? We need to clarify this before proceeding.Also how many
istiod
Pods are running per cluster? If it's only one, which is also exposed through a Service, then we should better useistiod.istio-system:15014
as host endpoint along with aleaderelection
condition. Similarly to what we have for cluster level metrics at https://github.com/elastic/integrations/blob/main/packages/kubernetes/data_stream/apiserver/manifest.yml#L21.If there are more than 1
istiod
Pod per cluster (for high availability maybe) then we should check if those keep and expose the same metrics or each one of them keep and provide different metrics (using a sharding mechanismo for example). In that case we could use the "autodiscovery" approach otherwise if the endpoint of the control plane metrics is unique we should not use the "autodiscovery" approach. See below:The way we have it now makes it more complicated and resource consuming since we use an "autodiscovery" approach when the endpoint is unique per cluster and static through a Service. This means that every time the
istiod
Pod's state is updated we will trigger an event that this Pod is updated and we will re-launch the "autodiscovery" event. This happens at https://github.com/elastic/elastic-agent/blob/main/internal/pkg/composable/providers/kubernetes/pod.go#L218-L232 and if you follow the codebase you can understand that the processing load is quite a lot.Add to this the fact that "autodiscovery" based inputs are harder to troubleshoot them.
Consequently it'snot a suggested practice to use "autodiscovery" conditions if not really needed and I would prefer fixing it at first place here instead of having a follow up.
Let me know if that makes sense or if I miss anything here.