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

Add envoy metrics discovery bundle #5780

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add envoy metrics discovery bundle #5780

wants to merge 7 commits into from

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Jan 11, 2025

Description:
Adds a new discovery bundle targeting envoy proxy metrics.

@atoulme atoulme requested review from a team as code owners January 11, 2025 00:50
@atoulme
Copy link
Contributor Author

atoulme commented Jan 11, 2025

I tested this locally. It worked well enough on my machine.

# rule:
# docker_observer: type == "container" and any([name, image, command], {# matches "(?i)envoy"}) and not (command matches "splunk.discovery")
# host_observer: type == "hostport" and command matches "(?i)envoy" and not (command matches "splunk.discovery")
# k8s_observer: type == "port" and pod.name matches "(?i)envoy"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the k8s rule intentionally matching just pod name instead of the broader rule used by docker_observer? Istio sidecars for e.g. have pod name like istio-proxy, the image name might have envoy in it. But maybe we should only match standalone envoy to startwith

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, you had a note about that. I think we can update the rule to add istio-proxy but then I really need a test showing this works for an istio mesh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am tempted to make a separate integration test with a kind cluster, we deploy everything and we make sure it works. I might get into that next and tweak the rule based on that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should merge this one as-is. And have a different item to officially support istio. It is not just the pod name; default prom port used by istio is also not same as the one here. Don't know if it is supported in deiscover mode, but it would be better getting the port/path from the prometheus.io annotations for istio (like we have in our helm chart).

@jinja2
Copy link
Contributor

jinja2 commented Jan 11, 2025

Discover mode config test TestDockerObserver is failing

        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -49,3 +49,25 @@
        	            	    (string) (len=26) "receiver_creator/discovery": (map[string]interface {}) (len=2) {
        	            	-    (string) (len=9) "receivers": (map[string]interface {}) (len=2) {
        	            	+    (string) (len=9) "receivers": (map[string]interface {}) (len=3) {
        	            	+     (string) (len=10) "prometheus": (map[string]interface {}) (len=3) {
        	            	+      (string) (len=6) "config": (map[string]interface {}) (len=1) {
        	            	+       (string) (len=6) "config": (map[string]interface {}) (len=1) {
        	            	+        (string) (len=14) "scrape_configs": ([]interface {}) (len=1) {
        	            	+         (map[string]interface {}) (len=5) {
        	            	+          (string) (len=8) "job_name": (string) (len=5) "envoy",
        	            	+          (string) (len=22) "metric_relabel_configs": ([]interface {}) (len=1) {
        	            	+           <max depth reached>
        	            	+          },
        	            	+          (string) (len=12) "metrics_path": (string) (len=17) "/stats/prometheus",
        	            	+          (string) (len=15) "scrape_interval": (string) (len=3) "10s",
        	            	+          (string) (len=14) "static_configs": ([]interface {}) (len=1) {
        	            	+           <max depth reached>
        	            	+          }
        	            	+         }
        	            	+        }
        	            	+       }
        	            	+      },
        	            	+      (string) (len=19) "resource_attributes": (map[string]interface {}) {
        	            	+      },
        	            	+      (string) (len=4) "rule": (string) (len=121) "type == \"container\" and any([name, image, command], {# matches \"(?i)envoy\"}) and not (command matches \"splunk.discovery\")"
        	            	+     },
        	            	      (string) (len=17) "prometheus_simple": (map[string]interface {}) (len=3) {
        	Test:       	TestDockerObserver

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

Successfully merging this pull request may close these issues.

3 participants