-
Notifications
You must be signed in to change notification settings - Fork 134
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
Wrap the server with the Prometheus so we get metrics + add an e2e te… #267
Conversation
…st for it. I wrapped it in a job, since otherwise it will be trickier to get access to the metrics from outside the kind cluster. Fix the dangling pointer from sigstore#262 and fix a tiny typo, though I kind of do like exposing a singingCert that might come handy in karaoke? Signed-off-by: Ville Aikas <[email protected]>
Signed-off-by: Ville Aikas <[email protected]>
Signed-off-by: Ville Aikas <[email protected]>
Signed-off-by: Ville Aikas <[email protected]>
lucky and hit the one that we didn't sign, so testing that. Signed-off-by: Ville Aikas <[email protected]>
Remove debug cruft. Signed-off-by: Ville Aikas <[email protected]>
Signed-off-by: Ville Aikas <[email protected]>
…nto it. Signed-off-by: Ville Aikas <[email protected]>
…268) * While working on #267 noticed this, but didn't want to bake into it. Signed-off-by: Ville Aikas <[email protected]> * Forgot one change, thanks @mattmoor :) Signed-off-by: Ville Aikas <[email protected]>
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.
lgtm
very cool
}) | ||
// Wrap the inner func with instrumentation to get latencies | ||
// that get partitioned by 'code' and 'method'. | ||
return promhttp.InstrumentHandlerDuration( |
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.
Will the decorate handler mean fulcio would still function correctly without a running Prometheus instance present?What would happen to requests in that scenario?
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.
The nice part of the prometheus model is that it's pull based - exposing metrics with no instance is safe and a noop. A running collector would be needed to actually scrape these and do something useful with them.
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.
gotcha, good for me.
Wrap the server with the Prometheus so we get metrics + add an e2e test for it. Note that
the default prometheus metrics (go_*) were already exposed.
I wrapped test in a job, since otherwise it will be trickier to get access to the
metrics from outside the kind cluster.
Fix the dangling pointer from #262 and fix a tiny typo, though I kind of do
like exposing a singingCert that might come handy (should be required?) in karaoke?
Signed-off-by: Ville Aikas [email protected]
Summary
Ticket Link
Fixes
Release Note