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

add support for prometheus metrics #25

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Conversation

Fijxu
Copy link
Contributor

@Fijxu Fijxu commented Dec 18, 2024

Useful for troubleshooting blockages in a visual manner using Grafana.

It doesn't count how many times poTokenGenerate has failed between lines 69-83, I didn't wanted to touch that much code. But I'll make a new commit to count on those lines too so you can review it.

@unixfox
Copy link
Member

unixfox commented Dec 18, 2024

Maybe you can includes some "default" dashboards :)

@unixfox
Copy link
Member

unixfox commented Mar 16, 2025

Could you allow this to be toggleable? I think people running private instances don't need it, it's more towards public instances.

@Fijxu Fijxu force-pushed the metrics branch 2 times, most recently from 64b46dc to 88c2296 Compare March 18, 2025 19:42
@Fijxu
Copy link
Contributor Author

Fijxu commented Mar 18, 2025

This should be better, tell me if you want to change or add something

Edit: I'll add the grafana dashboard JSON in a few minutes

Copy link
Collaborator

@alexmaras alexmaras left a comment

Choose a reason for hiding this comment

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

I'm going to pull this down and test it too, but here's some comments in the mean time. Nothing looks wildly problematic.

@alexmaras
Copy link
Collaborator

Just an update - pulled the branch and tested with prometheus and things look like they're correctly read in, so the format of the output all looks good.

Comment on lines 62 to 108
switch (true) {
// CONTENT_CHECK_REQUIRED: Sensitive content videos.
case (videoData.playabilityStatus?.status ===
"CONTENT_CHECK_REQUIRED"): {
break;
}
case (videoData.playabilityStatus?.status === "LOGIN_REQUIRED"): {
switch (true) {
// Age restricted videos, we don't need to track those.
case videoData.playabilityStatus?.reason?.includes(
"Sign in to confirm your age",
): {
break;
}

case videoData.playabilityStatus?.reason?.includes(
"Sign in to confirm you’re not a bot",
): {
this.innertubeErrorReasonSignIn.inc();

switch (true) {
case videoData.playabilityStatus?.errorScreen
?.playerErrorMessageRenderer
?.subreason?.runs?.[0]?.text?.includes(
"This helps protect our community",
): {
this.innertubeErrorSubreasonProtectCommunity
.inc();
break;
}
default: {
this.innertubeErrorSubreasonUnknown.inc();
break;
}
}

break;
}

default: {
this.innertubeErrorReasonUnknown.inc();
break;
}
}
break;
}
default:
this.innertubeErrorStatusUnknown.inc();
break;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is tricky, I'm going to have a play with this and see if I can find a nice way to do it that feels like it'll extend a bit more easily without becoming impossible to understand.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the only bit I'm stuck on now, the rest of the PR looks good to me. I'm playing around with some options, these kinds of logical mazes are always painful to parse and implement.

Copy link
Member

Choose a reason for hiding this comment

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

interested too in a "better" format

Copy link
Contributor Author

@Fijxu Fijxu Mar 27, 2025

Choose a reason for hiding this comment

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

Done, this looks better, unless you want to do the checking inside the own checkStatus, checkReason and checkSubreason functions.

Edit: It seems to work well, but I don't have any IP banned so I can't test the other reason and subreasons regarding ip blocks.

@unixfox
Copy link
Member

unixfox commented Mar 27, 2025

Please rebase thank you :)

@@ -15,6 +15,7 @@
# # secret key needs to be 16 characters long or more
# secret_key = "CHANGE_ME" # env variable: SERVER_SECRET_KEY
# verify_requests = false
# enable_metrics = false # env variable: ENABLE_METRICS
Copy link
Member

Choose a reason for hiding this comment

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

Same as #65, please rename to SERVER_ENABLE_METRICS

deno.json Outdated
@@ -6,6 +6,7 @@
"imports": {
"hono": "jsr:@hono/[email protected]",
"@std/toml": "jsr:@std/[email protected]",
"prom-client": "npm:[email protected]",
Copy link
Member

Choose a reason for hiding this comment

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

I would really like to avoid npm as much as possible, would it be possible for you to point to the github repo: https://github.com/siimon/prom-client/blob/v15.1.3/index.js?

You may use https://esm.sh to help you.

Using npm in deno requires enabling many many useless environment variables that I would like to get rid of at some point if we can stop using jsdom from npm.

@Fijxu
Copy link
Contributor Author

Fijxu commented Mar 27, 2025

I'll add a default Grafana dashboard, do not merge it yet ;)

Fijxu added 2 commits March 27, 2025 18:04
I got a warning from grafana saying that "metric may not be a counter,
name does not end with _total/_sum/ ...".

This changes the metric names using the prometheus naming convention
(https://prometheus.io/docs/practices/naming/)
enable_metrics: z.boolean().default(
Deno.env.get("SERVER_ENABLE_METRICS") === "true"
? true
: Deno.env.get("SERVER_ENABLE_METRICS") === "false"
Copy link
Member

Choose a reason for hiding this comment

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

Why do you check for enable metrics env to be false?

If you remove the parameter it's false by default.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, default of Deno.env.get("SERVER_ENABLE_METRICS") === "true" will make it a boolean correctly. At the moment it'd always be true unless the env variable SERVER_ENABLE_METRICS is "false"

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