-
Notifications
You must be signed in to change notification settings - Fork 8
Add Prometheus Metric QOL improvements to local env #198
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,13 +11,15 @@ services: | |
| - "${TEMPORAL_INTERNAL_PORT}" | ||
| - --ip | ||
| - "0.0.0.0" | ||
| - --headless | ||
| - --ui-port | ||
| - "${TEMPORAL_UI_INTERNAL_PORT}" | ||
| - --namespace | ||
| - default | ||
| - --log-level | ||
| - warn | ||
| ports: | ||
| - "${TEMPORAL_LEFT_EXTERNAL_PORT}:${TEMPORAL_INTERNAL_PORT}" | ||
| - "${TEMPORAL_LEFT_UI_EXTERNAL_PORT}:${TEMPORAL_UI_INTERNAL_PORT}" | ||
| networks: | ||
| - develop | ||
| healthcheck: | ||
|
|
@@ -43,13 +45,15 @@ services: | |
| - "${TEMPORAL_INTERNAL_PORT}" | ||
| - --ip | ||
| - "0.0.0.0" | ||
| - --headless | ||
| - --ui-port | ||
| - "${TEMPORAL_UI_INTERNAL_PORT}" | ||
| - --namespace | ||
| - default | ||
| - --log-level | ||
| - warn | ||
| ports: | ||
| - "${TEMPORAL_RIGHT_EXTERNAL_PORT}:${TEMPORAL_INTERNAL_PORT}" | ||
| - "${TEMPORAL_RIGHT_UI_EXTERNAL_PORT}:${TEMPORAL_UI_INTERNAL_PORT}" | ||
| networks: | ||
| - develop | ||
| healthcheck: | ||
|
|
@@ -75,6 +79,7 @@ services: | |
| volumes: *x-volumes | ||
| ports: | ||
| - "${PROXY_RIGHT_EXTERNAL_PORT}:${PROXY_RIGHT_INTERNAL_PORT}" | ||
| - "${PROXY_RIGHT_METRICS_EXTERNAL_PORT}:${PROXY_METRICS_INTERNAL_PORT}" | ||
| networks: | ||
| - develop | ||
| depends_on: | ||
|
|
@@ -83,7 +88,7 @@ services: | |
| healthcheck: # TODO: Replace with healthcheck endpoint. | ||
| test: | ||
| - CMD-SHELL | ||
| - "wget -q -O /dev/null -T 3 http://localhost:6060/debug/pprof/ || exit 1" | ||
| - "wget -q -O /dev/null -T 3 http://localhost:${PROXY_PPROF_INTERNAL_PORT}/debug/pprof/ || exit 1" | ||
| interval: 5s | ||
| timeout: 5s | ||
| retries: 30 | ||
|
|
@@ -98,6 +103,7 @@ services: | |
| volumes: *x-volumes | ||
| ports: | ||
| - "${PROXY_LEFT_EXTERNAL_PORT}:${PROXY_LEFT_INTERNAL_PORT}" | ||
| - "${PROXY_LEFT_METRICS_EXTERNAL_PORT}:${PROXY_METRICS_INTERNAL_PORT}" | ||
| networks: | ||
| - develop | ||
| depends_on: | ||
|
|
@@ -108,12 +114,27 @@ services: | |
| healthcheck: # TODO: Replace with healthcheck endpoint. | ||
| test: | ||
| - CMD-SHELL | ||
| - "wget -q -O /dev/null -T 3 http://localhost:6060/debug/pprof/ || exit 1" | ||
| - "wget -q -O /dev/null -T 3 http://localhost:${PROXY_PPROF_INTERNAL_PORT}/debug/pprof/ || exit 1" | ||
| interval: 5s | ||
| timeout: 5s | ||
| retries: 30 | ||
| start_period: 10s | ||
|
|
||
| prometheus: | ||
| image: prom/prometheus:latest | ||
| command: | ||
| - --config.file=/etc/develop/docker-compose/tmp/develop.prometheus.yaml | ||
| volumes: *x-volumes | ||
| ports: | ||
| - "${PROMETHEUS_EXTERNAL_PORT}:${PROMETHEUS_INTERNAL_PORT}" | ||
| networks: | ||
| - develop | ||
| depends_on: | ||
| proxy-left: | ||
| condition: service_healthy | ||
| proxy-right: | ||
| condition: service_healthy | ||
|
Comment on lines
+123
to
+136
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we want a prometheus server in here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Intent is to facilitate faster and more-reliable local testing, specifically for metrics - which is where I'm making changes next. There are 3 approaches I see:
I think Prometheus is faster to validate locally than exposing each There could be use case for Grafana in the future, but at the moment, I don't believe we need it. |
||
|
|
||
| smoke-test: | ||
| image: temporalio/temporal:1.5.0 | ||
| entrypoint: /bin/sh | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,18 @@ TEMPORAL_LEFT_EXTERNAL_PORT=4000 | |
| TEMPORAL_RIGHT_EXTERNAL_PORT=5000 | ||
| PROXY_LEFT_EXTERNAL_PORT=4001 | ||
| PROXY_RIGHT_EXTERNAL_PORT=5001 | ||
| PROXY_LEFT_METRICS_EXTERNAL_PORT=4091 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i thought we want to use different name other than left/right? not need for this PR. just want to understand if we still plan to do so.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. We will standardize - but outside scope of this PR. |
||
| PROXY_RIGHT_METRICS_EXTERNAL_PORT=5091 | ||
| PROMETHEUS_EXTERNAL_PORT=9090 | ||
| TEMPORAL_LEFT_UI_EXTERNAL_PORT=4233 | ||
| TEMPORAL_RIGHT_UI_EXTERNAL_PORT=5233 | ||
|
|
||
| # Internal Ports (exposed on containers in docker network) | ||
| TEMPORAL_INTERNAL_PORT=7233 | ||
| TEMPORAL_UI_INTERNAL_PORT=8233 | ||
| PROXY_LEFT_INTERNAL_PORT=6233 | ||
| PROXY_RIGHT_INTERNAL_PORT=6333 | ||
| PROXY_MUX_INTERNAL_PORT=6334 | ||
| PROXY_PPROF_INTERNAL_PORT=6060 | ||
| PROXY_METRICS_INTERNAL_PORT=9091 | ||
| PROMETHEUS_INTERNAL_PORT=9090 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| global: | ||
| scrape_interval: 15s | ||
|
|
||
| scrape_configs: | ||
| - job_name: proxy-left | ||
| static_configs: | ||
| - targets: ["proxy-left:${PROXY_METRICS_INTERNAL_PORT}"] | ||
|
|
||
| - job_name: proxy-right | ||
| static_configs: | ||
| - targets: ["proxy-right:${PROXY_METRICS_INTERNAL_PORT}"] |
Uh oh!
There was an error while loading. Please reload this page.
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.
Opted to derive from env file as source of truth. Result is a more readable, and links are
cmd+clickable:The "more-correct" approach would be to derive from running containers directly - something like
docker compose ps --format "table {{.Service}}\t{{.Ports}}"But the result is actually less-usable and descriptive imo.
The downside of implemented approach is that the make implementation isn't particularly easy to read, and isn't deriving from more reliable source of truth.