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 remote-write to Prombench #787

Merged
merged 3 commits into from
Nov 27, 2024

Conversation

kushalShukla-web
Copy link
Contributor

This PR introduces a Deployment and a Service to test the Prometheus Remote Write functionality.

Currently, the image for the Prometheus sink is not applied in the YAML file. To address this, I have created a basic YAML configuration that sets up the necessary Deployment and Service to test the Prometheus Remote Write functionality.

  1. Deployment: A basic deployment is created to run the Prometheus sink container (quay.io/bwplotka/sink:latest). The container exposes port 9011 as sink-port.
  2. Service: A headless ClusterIP Service is defined to expose port 80 and forward traffic to the sink-port on the pods.

@bboreham bboreham changed the title Created Service and Deployment configuration for Prometheus sink Add remote-write to Prombench Nov 18, 2024
Copy link
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

This is very good overall. Couple of improvements I can suggest.

apiVersion: v1
kind: Service
metadata:
name: prometheus-remote-write
Copy link
Member

Choose a reason for hiding this comment

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

I think it might be easier if this were named the same as the Deployment.
That's the pattern used for fake-webserver, prometheus-loadgen-querier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay.

name: prometheus-remote-write
namespace: prombench-{{ .PR_NUMBER }}
labels:
app: remote-write
Copy link
Member

Choose a reason for hiding this comment

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

This should be consistent with other app labels in this file.
That's the point, to group them all together.

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 did not knew that.

spec:
ports:
- name: prometheus
port: 80
Copy link
Member

Choose a reason for hiding this comment

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

Why? (It doesn't really matter, but I would have stuck with 9011)

app: remote-write
spec:
ports:
- name: prometheus
Copy link
Member

Choose a reason for hiding this comment

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

This seems potentially confusing; it's not a Prometheus.

@@ -718,3 +718,5 @@ data:
- action: replace
source_labels: [__meta_kubernetes_pod_node_name]
target_label: nodeName
remote_write:
- url: "http://prometheus-remote-write:80/sink/prw"
Copy link
Member

Choose a reason for hiding this comment

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

Please resolve the red mark.

@bboreham
Copy link
Member

headless ClusterIP Service

Services are either headless or ClusterIP, not both. See https://kubernetes.io/docs/concepts/services-networking/service/#type-clusterip

2. make clusterIp headless.
Signed-off-by: Kushal Shukla <[email protected]>
Comment on lines 21 to 23
limits:
cpu: "1"
memory: "350Mi"
Copy link
Member

Choose a reason for hiding this comment

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

Leave off limits until we know more about what it needs.
If it hits the memory limit it will crash, which will be annoying.

Signed-off-by: Kushal Shukla <[email protected]>
Copy link
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Seems good enough to try.

app: sink
template:
metadata:
namespace: prombench-{{ .PR_NUMBER }}
Copy link
Member

Choose a reason for hiding this comment

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

Normally containers inherit the namespace from the Deployment.
Maybe it will work.

@bboreham bboreham merged commit aca1803 into prometheus:master Nov 27, 2024
3 checks passed
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.

2 participants