-
Notifications
You must be signed in to change notification settings - Fork 24
metrics: track unidentified innertube errors #107
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
base: master
Are you sure you want to change the base?
Conversation
36d1473
to
e030ebb
Compare
feb0bb0
to
c5b1ccc
Compare
Note to self:
|
352fecb
to
fb92354
Compare
I'll leave it as a draft for now since I plan to refactor some things and I still need to add some metrics |
if (googlevideoResponse.status == 403) { | ||
metrics?.videoplaybackForbidden.labels({ | ||
method: "HEAD", | ||
}).inc(); | ||
} |
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.
we could track all status codes, but seems overkill to me. I've personally never seen any other http status code more than 403
when the request to /videoplayback
fails for any reason
We can't track
We would need to catch that error somehow in: invidious-companion/src/lib/helpers/youtubePlayerHandling.ts Lines 54 to 58 in f9f89a2
Or maybe override the |
Note to self:
|
bad177b
to
b1eb0b2
Compare
Now it should track |
8f1ebe1
to
32b1286
Compare
We should filter the errors by the client used? Because since we switched to For example,
meanwhile
Meanwhile, I'm going to adapt it so it can search for the reason on every JSON object, without client filtering. In my opinion, having a label or some way to display which client was used to display the unknown error would be better. |
85ea2c1
to
94a07f5
Compare
d109734
to
1daa09e
Compare
@Fijxu the fallback clients shouldn't be tried if there are no streamingData returned, that's what a private video do. There are no streams URLs returned since it's a private video. Or maybe my algorithm is wrong. On those cases, only WEB should be tried. |
You're right 😅 I did that because I didn't even read 16d06cd at the time you pushed it. When I was adding those type of errors, I used I'll delete those that I added because of |
2411ef7
to
299765b
Compare
9c003ae
to
d6d6c25
Compare
I consider this ready to merge. It has been working pretty well from my testing. My metrics are public so here is how they look like: |
6967726
to
f8296a5
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.
Pull Request Overview
This PR implements metrics tracking for unidentified InnerTube errors to improve monitoring and debugging capabilities. The changes expand error categorization and add tracking for video playback failures and player deciphering errors.
- Enhanced metrics system to support labeled counters for better error categorization
- Added comprehensive error tracking for InnerTube responses including status, reason, and subreason classification
- Implemented tracking for video playback 403 errors and YouTube.js player deciphering failures
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
src/routes/videoPlaybackProxy.ts | Adds metrics tracking for 403 forbidden responses from YouTube's videoplayback endpoint |
src/lib/helpers/youtubePlayerHandling.ts | Implements error tracking for player deciphering failures with try-catch wrapper |
src/lib/helpers/metrics.ts | Major refactoring to support labeled metrics and comprehensive InnerTube error categorization |
grafana_dashboard.json | Updates dashboard configuration to display new metrics with improved visualization |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Added reason and subreason for sentitive content videos ("CONTENT_CHECK_REQUIRED") fix: fix typo: `SignInToConfirmBot` -> `signInToConfirmBot`
… video unavailable errors
Some subreaons of video unavailable may be missing, those will be added in subsequent commits.
…WEB and TV client
de06565
to
43f877f
Compare
Closes #103 and #151
The second commit will contain fixes and new errors that are not directly related to #103, so if it's possible, don't squash this PR, otherwise, I can create a new PR after this gets merged