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

update tf charm to include metrics endpoint - Prometheus #133

Closed
wants to merge 4 commits into from

Conversation

hum4n0id
Copy link
Contributor

Description

Expose Prometheus metrics endpoint for COS lite

Resolved issues

https://warthogs.atlassian.net/browse/CERTTF-203

Tests

Tested via Juju / microK8s

Added "api_port" to config.yaml to make the multiple references to this API port dynamic, helpful with add'l references for the metrics endpoint.

Copy link
Collaborator

@plars plars left a comment

Choose a reason for hiding this comment

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

You'll see that I also pushed a few changes in here to make the tests pass again, and add the missing charm libraries. There's one workflow still failing, but that one fails because you pushed this to your repo instead of the canonical one. Just make sure to push it to the canonical one next time

@@ -7,3 +7,7 @@ options:
default: 10
description: Number of seconds to wait for keepalive connections
type: int
api_port:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this section, we don't need api_port in the config, and it was actually recommended that we do not do this. Hardcoding things like this may seem unusual, but there's really no reason for it to change, and removing the option reduces the chances for config mismatches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem

jobs=[
{
"static_configs": [
{"targets": [f"*:{self.config['api_port']}"]}
Copy link
Collaborator

Choose a reason for hiding this comment

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

make sure to also remove the config item here

@@ -80,7 +94,7 @@ def _require_nginx_route(self):
charm=self,
service_hostname=self.config["external_hostname"],
service_name=self.app.name,
service_port=5000,
service_port=self.config["api_port"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

...and here

@@ -162,7 +176,7 @@ def _pebble_layer(self):
"--keep-alive",
keepalive,
"--bind",
"0.0.0.0:5000",
f"0.0.0.0:{self.config['api_port']}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

...and here

@hum4n0id hum4n0id closed this Oct 25, 2023
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