-
Notifications
You must be signed in to change notification settings - Fork 905
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
feat: Add prometheus and grafana support #523
Conversation
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.
👍 Looks good to me! Reviewed everything up to 86a670f in 19 seconds
More details
- Looked at
139
lines of code in6
files - Skipped
1
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. prometheus/prometheus_data/prometheus.yml:29
- Draft comment:
Ensure that the 'agents-api' service is running on port 8080, as specified in the Prometheus configuration. Otherwise, update the target to match the actual port. - Reason this comment was not posted:
Comment did not seem useful.
2. grafana/docker-compose.yml:8
- Draft comment:
Ensure that the environment variablesGRAFANA_ADMIN_PASSWORD
andGRAFANA_ADMIN_USER
are defined in your environment or in an.env
file to avoid issues with Grafana startup. - Reason this comment was not posted:
Comment did not seem useful.
3. prometheus/docker-compose.yml:11
- Draft comment:
Consider changing the host port from 11000 to 9090 to match the Prometheus default port, or update the Prometheus configuration to reflect the port mapping. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is suggesting a change that is not necessarily required. The port mapping of 11000:9090 is valid and could be intentional to avoid conflicts with other services using port 9090 on the host. The comment does not point out a definite issue but rather suggests a preference, which is not actionable or necessary.
I might be overlooking a standard practice or convention that requires the host port to match the container port, but this is not typically enforced. The comment could be highlighting a potential oversight if the user did not intend to change the host port.
The port mapping is a common practice to avoid port conflicts, and there is no strong evidence that this is an error. The comment does not provide a compelling reason to change the port mapping.
The comment should be deleted as it does not point out a definite issue and the current port mapping could be intentional.
Workflow ID: wflow_gCyKtoa3Cab7Bn9S
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
Skipped PR review on 49078df because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
Generated with ❤️ by ellipsis.dev
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.
👍 Looks good to me! Incremental review on 7404516 in 9 seconds
More details
- Looked at
57
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. prometheus/prometheus_data/prometheus.yml:29
- Draft comment:
Ensure that the agents-api service is correctly configured to run on port 8000. This change should be consistent with the actual service configuration. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_xJPtc3IOAErzCvI6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
❌ Changes requested. Incremental review on 5d30b05 in 17 seconds
More details
- Looked at
10
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_zwaq8Yw7uQllwmUY
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
scrape_interval: 10s | ||
metrics_path: /metrics | ||
static_configs: | ||
- targets: ['http://agents-api-multi-tenant:8080'] |
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.
Remove the protocol from the target URL. It should be just agents-api-multi-tenant:8080
without http://
. This applies to Prometheus configurations to ensure proper scraping.
5d30b05
to
124ecc7
Compare
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.
❌ Changes requested. Incremental review on 124ecc7 in 14 seconds
More details
- Looked at
10
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_A1gZPgrspJAvFoea
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
scrape_interval: 10s | ||
metrics_path: /metrics | ||
static_configs: | ||
- targets: ['http://agents-api-multi-tenant:8080'] |
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.
Remove 'http://' from the target URL. Prometheus expects just the hostname and port in the 'targets' list.
- targets: ['http://agents-api-multi-tenant:8080'] | |
- targets: ['agents-api-multi-tenant:8080'] |
45736c2
to
b13cb6b
Compare
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.
👍 Looks good to me! Reviewed everything up to 46d9369 in 25 seconds
More details
- Looked at
153
lines of code in7
files - Skipped
1
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. prometheus/config/prometheus.yml:24
- Draft comment:
Ensure that the 'agents-api-multi-tenant' service is defined and running, or update the target to match the actual service name and port. - Reason this comment was not posted:
Comment did not seem useful.
2. grafana/docker-compose.yml:8
- Draft comment:
Ensure that the environment variables 'GRAFANA_ADMIN_PASSWORD' and 'GRAFANA_ADMIN_USER' are defined in your environment or in an .env file. - Reason this comment was not posted:
Comment did not seem useful.
3. prometheus/docker-compose.yml:15
- Draft comment:
Ensure that the 'agents-api-multi-tenant' service is defined in your docker-compose setup or update the dependency to match the actual service name. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_rnRwaHsw4iYlP0xs
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Add Prometheus and Grafana support with FastAPI metrics and Docker Compose configurations.
prometheus-fastapi-instrumentator
toweb.py
for FastAPI metrics.pyproject.toml
to includeprometheus-fastapi-instrumentator
dependency.prometheus/docker-compose.yml
for Prometheus configuration.grafana/docker-compose.yml
for Grafana configuration.docker-compose.yml
to include Prometheus and Grafana services.traefik.yml.template
to route/grafana
to Grafana service.This description was created by for 46d9369. It will automatically update as commits are pushed.