-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add metric to track how many times is each key requested #24
Add metric to track how many times is each key requested #24
Conversation
25293b8
to
15ef058
Compare
I have a concern with cardinality here. We usually try not to add prometheus labels with unbounded cardinality, due to each unique label creating a unique prometheus timeseries. I guess the kids will be short-lived ~1 week, so this might be fine. |
@fisherdarling cardinality of key_id is 256 (from 0 to 255). I agree this is large. |
@lbaquerofierro I've posted a change proposal to your repo lbaquerofierro#1 It does not create an additional metric, and allows to set default labels. |
src/errors.ts
Outdated
@@ -11,7 +11,7 @@ function shouldSendToSentry(error: Error): boolean { | |||
export async function handleError(ctx: Context, error: Error) { | |||
console.error(error.stack); | |||
|
|||
ctx.metrics.erroredRequestsTotal.inc({ env: ctx.env.ENVIRONMENT }); | |||
ctx.metrics.erroredRequestsTotal.inc({ env: ctx.env.ENVIRONMENT, servcice: ctx.env.SERVICE }); |
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.
ctx.metrics.erroredRequestsTotal.inc({ env: ctx.env.ENVIRONMENT, servcice: ctx.env.SERVICE }); | |
ctx.metrics.erroredRequestsTotal.inc({ env: ctx.env.ENVIRONMENT, service: ctx.env.SERVICE }); |
src/index.ts
Outdated
ctx.metrics.signedTokenTotal.inc({ env: ctx.env.ENVIRONMENT, service: ctx.env.SERVICE }); | ||
|
||
const keyId = tokenRequest.truncatedTokenKeyId.toString(); | ||
ctx.metrics.keyRequestTotal.inc({ | ||
env: ctx.env.ENVIRONMENT, | ||
service: ctx.env.SERVICE, | ||
key_id: keyId, | ||
}); |
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.
only use this metric
ctx.metrics.signedTokenTotal.inc({ env: ctx.env.ENVIRONMENT, service: ctx.env.SERVICE }); | |
const keyId = tokenRequest.truncatedTokenKeyId.toString(); | |
ctx.metrics.keyRequestTotal.inc({ | |
env: ctx.env.ENVIRONMENT, | |
service: ctx.env.SERVICE, | |
key_id: keyId, | |
}); | |
const keyId = keyId = tokenRequest.truncatedTokenKeyId.toString(); | |
ctx.metrics.signedTokenTotal.inc({ env: ctx.env.ENVIRONMENT, service: ctx.env.SERVICE, key_id: keyId }); |
Great! Thanks for walking through the true cardinality. 14 is perfectly fine. |
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.
change looks ok.
once you've addressed the nit, can you squash your changes, and rebase your branch on origin/main
package.json
Outdated
@@ -40,7 +40,7 @@ | |||
"@typescript-eslint/parser": "6.21.0", | |||
"commander": "12.1.0", | |||
"dotenv": "16.4.0", | |||
"esbuild": "0.19.12", | |||
"esbuild": "^0.21.5", |
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.
nit: fix the version
968eecd
to
59de7f2
Compare
…inf default env vars
e4252fc
to
9bc8289
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.
I proposed one additional commit lbaquerofierro#2 that you should be able to merge.
Once done, we can rebase and merge this branch
Add SERVICE in wrangler.toml
No description provided.