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

Report the running version as a metric #163

Closed
wants to merge 15 commits into from
Closed

Conversation

DMRobertson
Copy link
Contributor

Allows one to display (or superimpose) the version onto grafana graphs, to cross-reference with deployments.

Allows one to display (or superimpose) the version onto grafana graphs,
to cross-reference with deployments.
Comment on lines 120 to 121
Namespace: "sliding-sync",
Name: "build-info",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These should be underscores. If not, runtime error.

Need to enable prometheus metrics endpoint in the E2E tests.

Copy link
Member

Choose a reason for hiding this comment

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

We have an entire file called metrics_test.go in integration which do this.

Copy link
Member

Choose a reason for hiding this comment

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

Ah but it hooks in further down the stack when making the handlers, not in the cmd binary.

Copy link
Member

Choose a reason for hiding this comment

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

This could be as trivial as starting the binary with the SYNCV3_PROM env var set then ensuring /metrics 200 OKs.

Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

Pending E2E tests to check it works.

@DMRobertson DMRobertson requested a review from kegsay August 2, 2023 12:14
Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

This isn't what I was expecting. I would rather we didn't bundle this with end-to-end tests, as it just complicates things.

I was expecting more a simple metrics job which built, ran and curled /metrics to verify that it returns some data.

@kegsay
Copy link
Member

kegsay commented Aug 2, 2023

For example, we currently have:

            - name: Check /client works
              run: ./tests-e2e/client-check.sh
              env:
                  SYNCV3_DB: user=postgres dbname=syncv3 sslmode=disable password=postgres host=localhost
                  SYNCV3_SERVER: https://matrix-client.matrix.org
                  SYNCV3_SECRET: itsasecret

where client-check.sh is:

#!/bin/bash -eu
export SYNCV3_BINDADDR=0.0.0.0:8844

# Run the binary and stop it afterwards.
./syncv3 &
SYNCV3_PID=$!
trap "kill $SYNCV3_PID" EXIT

# wait for the server to be listening, we want this endpoint to 404 instead of connrefused
until [ \
  "$(curl -s -w '%{http_code}' -o /dev/null "http://localhost:8844/idonotexist")" \
  -eq 404 ]
do
  echo 'Waiting for server to start...'
  sleep 1
done

echo 'Checking client is reachable...'
curl -f -LI "http://localhost:8844/client/"

We could easily extend this to check /metrics.

@DMRobertson DMRobertson requested a review from kegsay August 2, 2023 17:20
@DMRobertson
Copy link
Contributor Author

DMRobertson commented Aug 2, 2023

Oh, I've only just noticed the word

extend

in your comment. I can make this one script if you'd prefer.

@DMRobertson
Copy link
Contributor Author

Gonna drop this for now.

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