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

feat: [sc-10649] Expose Metrics in Ghost #201

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

Conversation

teghnet
Copy link
Member

@teghnet teghnet commented Feb 14, 2025

paulohpigatto
paulohpigatto previously approved these changes Feb 14, 2025
@teghnet teghnet marked this pull request as draft February 14, 2025 17:43
@teghnet
Copy link
Member Author

teghnet commented Feb 14, 2025

pending some discussions
cc: @MDobak

@teghnet teghnet marked this pull request as ready for review February 20, 2025 14:08
Copy link
Member

@WesleyCharlesBlake WesleyCharlesBlake left a comment

Choose a reason for hiding this comment

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

the tor-proxy chart is not used any more since we handle the deployment of tor/onion resources using the CRD's. please remove the tor-proxy changes from the pr.

Then to get ghost metrics port, we can update this

    - port:
        number: 8888
      backend:
        service:
          name: ghost
          port:
            number: {{ .Values.ghost.service.ports.webapi.port }}
    - port:
        number: 9999
      backend:
        service:
          name: {{ include "validator.fullname" . }}-metrics
          port:
            number: 9090

@WesleyCharlesBlake
Copy link
Member

oh and we would need to update the metrics service

@teghnet
Copy link
Member Author

teghnet commented Feb 20, 2025

  1. We are not exposing 9999 on TOR (yet?)
  2. We separate metrics from healthchecks.
  3. Healthchecks = livez + readyz
  4. Port 9100 is for healthchecks, meaning: both liveness and readyness

@@ -17813,7 +17813,7 @@ spec:
imagePullPolicy: Always
livenessProbe:
httpGet:
path: /healthz
path: /livez
Copy link
Member Author

Choose a reason for hiding this comment

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

because healthz is deprecated

@@ -169,9 +169,6 @@ ghost:
# - path: /webapi
Copy link
Member Author

Choose a reason for hiding this comment

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

It is very unlikely that we would expose webapi here.
As an example, it's OK, but the metrics below are configured elswhere and keeping them here would confuse folks.

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