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

CON-181 Add prometheus metrics to CN #3166

Merged
merged 25 commits into from
Jun 14, 2022
Merged

CON-181 Add prometheus metrics to CN #3166

merged 25 commits into from
Jun 14, 2022

Conversation

SidSethi
Copy link
Contributor

@SidSethi SidSethi commented May 27, 2022

Description

Adds prometheus integration to CN
Adds default metric collection
Adds duration metric collection for all requests (at least all that use handleResponse, which is not all)

Default metrics subset
Screen Shot 2022-05-27 at 11 43 01 AM

Custom metrics
Screen Shot 2022-06-13 at 5 16 20 PM

Tests

Added test coverage for /prometheus_metrics endpoint
Existing CN + maddog test coverage ensures other metric generation works correctly.

How will this change be monitored? Are there sufficient logs?

Can check metrics at any CN's /prometheus_metrics endpoint, or check charts in Prometheus & Grafana (not yet configured).

@dmanjunath
Copy link
Contributor

For reference, linking the other content node prometheus PR https://github.com/AudiusProject/audius-protocol/pull/2888/files

Copy link
Contributor

@dmanjunath dmanjunath left a comment

Choose a reason for hiding this comment

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

thanks for coming up with this POC. much simpler integration than the other but i still think we're missing some insights on how to best integrate this. i had some questions about how the metrics are instrumented and collected

@SidSethi SidSethi changed the title Add prometheus (metrics) to CN + duration metrics to all requests CON-181 Add prometheus (metrics) to CN + duration metrics to all requests May 31, 2022
@SidSethi SidSethi changed the title CON-181 Add prometheus (metrics) to CN + duration metrics to all requests CON-181 Add prometheus (metrics) to CN + duration metrics to all requests (from sid) May 31, 2022
creator-node/src/routes/prometheusMetricsRoutes.js Outdated Show resolved Hide resolved
creator-node/src/apiHelpers.js Outdated Show resolved Hide resolved
creator-node/src/routes/prometheusMetricsRoutes.js Outdated Show resolved Hide resolved
creator-node/src/apiHelpers.js Outdated Show resolved Hide resolved
@SidSethi SidSethi changed the title CON-181 Add prometheus (metrics) to CN + duration metrics to all requests (from sid) CON-181 v2 Add prometheus (metrics) to CN + duration metrics to all requests Jun 1, 2022
@SidSethi SidSethi changed the title CON-181 v2 Add prometheus (metrics) to CN + duration metrics to all requests CON-181 - sid - Add prometheus (metrics) to CN + duration metrics to all requests Jun 3, 2022
@gitguardian
Copy link

gitguardian bot commented Jun 7, 2022

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request
GitGuardian id Secret Commit Filename
3509188 Generic High Entropy Secret c16d74d .circleci/config.yml View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

@pull-request-size pull-request-size bot added size/L and removed size/M labels Jun 7, 2022
Copy link
Contributor Author

@SidSethi SidSethi left a comment

Choose a reason for hiding this comment

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

@joaquincasares @dmanjunath i know this took me a long time but i spent a while with the code + docs, thinking through best patterns. i responded to all your comments with my thoughts
also pushed up:

  • more fleshed out metric creation pattern
  • added a more complex metric to /tracks, per @joaquincasares 's example/suggestion
  • added gauge metrics to understand their API + ensure it fits in my proposed pattern
  • first pass at getting test coverage

pls keep in mind that PR is still in draft and code is not ready for full review, just take a look at the high level patterns

Copy link
Contributor

@joaquincasares joaquincasares left a comment

Choose a reason for hiding this comment

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

Looking good! 🔥

creator-node/src/apiHelpers.js Outdated Show resolved Hide resolved
creator-node/src/apiHelpers.js Outdated Show resolved Hide resolved
creator-node/src/routes/tracks.js Show resolved Hide resolved
Copy link
Contributor

@dmanjunath dmanjunath left a comment

Choose a reason for hiding this comment

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

i like this approach a lot. very light and very flexible. we can add more structure if we want later on

creator-node/src/routes/tracks.js Show resolved Hide resolved
creator-node/test/audiusUsers.test.js Outdated Show resolved Hide resolved
@SidSethi SidSethi requested a review from joaquincasares June 10, 2022 15:57
@SidSethi SidSethi marked this pull request as ready for review June 10, 2022 15:57
Copy link
Contributor

@joaquincasares joaquincasares left a comment

Choose a reason for hiding this comment

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

Looks good!

I notice the Draft label was removed. Is this the final review now?

If so, I went through it a few times and everything looks good! I'll come back to it first thing Monday with fresh eyes, but I don't see any blockers at the moment!

@@ -131,8 +131,22 @@ const healthCheckController = async (req) => {
* Controller for `health_check/sync` route, calls
* syncHealthCheckController
*/
const syncHealthCheckController = async () => {
const syncHealthCheckController = async (req) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i just used this as an example of a Gauge metric - the previous example in Monitors was actually broken

const { createBullBoard } = require('@bull-board/api')
const { BullAdapter } = require('@bull-board/api/bullAdapter')
const { ExpressAdapter } = require('@bull-board/express')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just moved lines from below

Copy link
Contributor

@dmanjunath dmanjunath left a comment

Choose a reason for hiding this comment

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

I think this looks great. The README and examples are very clear as well

@SidSethi SidSethi requested a review from joaquincasares June 13, 2022 23:54
Copy link
Contributor

@joaquincasares joaquincasares left a comment

Choose a reason for hiding this comment

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

This looks great!

Aside from the one line deletion and the _counts_total thread, everything here looks golden! 🔥

@SidSethi SidSethi merged commit c29e7ae into master Jun 14, 2022
@SidSethi SidSethi deleted the ss-cn-prom branch June 14, 2022 16:45
sliptype pushed a commit that referenced this pull request Sep 10, 2023
[eb23ea0] [C-2309] Fix track title hover state (#3197) Dylan Jeffers
[b82a6ee] [C-2303] Ensure consistent play/active state for entity tiles (#3195) Dylan Jeffers
[3faedfb] [C-2223] Flex on Finnuh (#3196) Raymond Jacobson
[d1cd884] [C-2271] Update data passed to edit playlist form to update playlist properly (#3183) Kyle Shanks
[7837460] [C-2187] Improve sign-up error messages (#3194) Dylan Jeffers
[e200477] [C-2287] Update wallet connect language, flags, icons (#3192) Dylan Jeffers
[cc0b56d] allow create notification to handle playlist id as array or int (#3190) sabrina-kiam
[d357b3b] [C-2431] Add tastmaker notification to mobile (#3191) Dylan Jeffers
[83a6d36] No playlists text fix C-2296 (#3189) nicoback2
[0e44281] [PLAT-847]: Fix user subscription create album notification copy (#3162) sabrina-kiam
[7da3e10] [C-2412] Opaque id deeplinks (#3188) Sebastian Klingler
[4ff1c53] [C-2418] Fix mobile web open in app banner (#3173) Raymond Jacobson
[22a1240] [C-1504] Prevent user from following themselves (#3187) Dylan Jeffers
[465182a] [C-2306] Fix sign-in sign-up invalid email bug (#3186) Dylan Jeffers
[2cec764] [C-1750] Refactor top tags (#3180) Dylan Jeffers
[7ca864f] Fix Snap sharing with weird track names C-2081 (#3176) nicoback2
[761187a] Fix missing track on Sammie's page C-2342 (#3182) nicoback2
[41c9cf5] Fix track upload cover art name (#3138) Theo Ilie
[cfb0b96] [C-2428] Handle null in cache map handle/permalink adds (#3184) Andrew Mendelsohn
[0559902] [PAY-1127] Improve mobile chats performance: remove index dep (#3170) Reed
[961649a] Use RNGH touchables for mobile chats (#3174) Reed
[be5d2a5] [C-2427] Fix preinstall checks for ruby and python versions (#3179) Andrew Mendelsohn
[48f8d4c] Update readme with deps, remove sudo (#3178) Reed
[fe79400] Disallow sharing premium content to social media, remove feature flags C-2334 C-2420 (#3172) nicoback2
[776596f] [PAY-829][PAY-1044] Desktop: Chats search users modal permissions/blocks and profile page permissions/blocks (#3156) Marcus Pasell
[e4df304] Fix scrolling on ChatListScreen (#3171) Reed
[6f35699] [PAY-1041][PAY-1040][PAY-1126] Mobile search users screen improvements (#3159) Reed
[d9e302b] [PAY-1143] Desktop: Overflow menu on Chat Header (#3169) Marcus Pasell
[798a664] Fix edit profile button not working after you submit the first time (#3163) nicoback2
[1d78774] [PLAT-814] Use sdk discovery-node-selection in libs (#3125) Dylan Jeffers
[58005c4] [PAY-799] DMs: Link previews (#3096) Marcus Pasell
[7e3d44c] [PAY-924][Desktop] DMs: Blocking/Unblocking Wire Up/UI (#3155) Marcus Pasell
[7038d81] [C-2347] Improve popover zIndex (#3168) Dylan Jeffers
[3eb62fe] [C-2364] Fix missing artist-card badges on android (#3165) Dylan Jeffers
[64ffb7f] Update @audius/sdk to 2.0.3-beta.4 (#3160) Marcus Pasell
[6e5306c] [QA-443] Fix cache case matching for handles and permalinks (#3167) Andrew Mendelsohn
[84dcb5a] [C-2059] Show toast when email check fails due to blocked user (#3166) Sebastian Klingler
[be855a4] [C-2419] Fix offline lineup loading (#3164) Andrew Mendelsohn
[920e25b] Fix permissions message for adding to photos C-2379 (#3161) nicoback2
[78cbdfa] [PLAT-819]: Fix copy for repost and save of albums notifications (#3151) sabrina-kiam
[10617a2] Mobile chats use push instead of navigate (#3154) Reed
[6131075] [C-2406] Fix repeated edits of same track (#3157) Sebastian Klingler
[3ae27d1] Mobile chats scrolling improvements (#3153) Reed
[e820e3f] [C-2336, C-2388] Playlist style and bug fixes (#3149) Kyle Shanks
[955887f] [C-2406] Null check localStorage (#3150) Sebastian Klingler
[3b31b73] [C-2227] Clear wallet state on sign-out (#3147) Dylan Jeffers
[91940fb] [C-2406] Prevent non finite set of currentTime (#3148) Sebastian Klingler
[d799e30] [C-2404] Fix ios build on xcode 14.3 (#3146) Dylan Jeffers
[186a086] [C-2398] Clean up playlist-update feature flags (#3145) Dylan Jeffers
[edb3142] update to xcode 14.2.0 (#3107) Raymond Jacobson
[4add953] [PLAT-835] Fix tastemaker twitter button (#3144) Dylan Jeffers
[e68ada9] [PLAT-834]: Fix notifications last seen new tag (#3143) sabrina-kiam
@AudiusProject AudiusProject deleted a comment from linear bot Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants