Skip to content

CON-181 Add prometheus metrics to CN - #3166

Merged
SidSethi merged 25 commits into
masterfrom
ss-cn-prom
Jun 14, 2022
Merged

CON-181 Add prometheus metrics to CN#3166
SidSethi merged 25 commits into
masterfrom
ss-cn-prom

Conversation

@SidSethi

@SidSethiSidSethi commented May 27, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

@dmanjunathdmanjunath left a comment

Copy link
Copy Markdown
Contributor

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

Comment threadcreator-node/src/services/prometheusMonitoring/prometheusRegistry.js Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/prometheusRegistry.js Outdated
Comment threadcreator-node/src/apiHelpers.js Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/prometheusRegistry.js Outdated
@SidSethiSidSethi changed the title Add prometheus (metrics) to CN + duration metrics to all requestsCON-181 Add prometheus (metrics) to CN + duration metrics to all requestsMay 31, 2022
@SidSethiSidSethi changed the title CON-181 Add prometheus (metrics) to CN + duration metrics to all requestsCON-181 Add prometheus (metrics) to CN + duration metrics to all requests (from sid)May 31, 2022
Comment threadcreator-node/src/services/prometheusMonitoring/prometheusRegistry.js Outdated
Comment threadcreator-node/src/routes/prometheusMetricsRoutes.js Outdated
Comment threadcreator-node/src/apiHelpers.js Outdated
Comment threadcreator-node/src/routes/prometheusMetricsRoutes.js Outdated
Comment threadcreator-node/src/apiHelpers.js Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/prometheusRegistry.js Outdated
Comment threadcreator-node/src/apiHelpers.js Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/prometheusRegistry.js Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/prometheusRegistry.js Outdated
@SidSethiSidSethi 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 requestsJun 1, 2022
@SidSethiSidSethi changed the title CON-181 v2 Add prometheus (metrics) to CN + duration metrics to all requestsCON-181 - sid - Add prometheus (metrics) to CN + duration metrics to all requestsJun 3, 2022
@gitguardian

gitguardianBot commented Jun 7, 2022

Copy link
Copy Markdown

⚠️ 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 idSecretCommitFilename
3509188Generic High Entropy Secretc16d74d.circleci/config.ymlView 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!

@SidSethiSidSethi left a comment

Copy link
Copy Markdown
ContributorAuthor

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

Comment threadcreator-node/src/services/prometheusMonitoring/prometheusRegistry.js Outdated
Comment threadcreator-node/src/routes/prometheusMetricsRoutes.js Outdated
Comment threadcreator-node/src/routes/prometheusMetricsRoutes.js Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/prometheusRegistry.js Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/prometheusRegistry.js Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/prometheusRegistry.js Outdated
Comment threadcreator-node/src/apiHelpers.js Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/prometheusRegistry.js Outdated

@joaquincasaresjoaquincasares left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking good! 🔥

Comment threadcreator-node/src/apiHelpers.js Outdated
Comment threadcreator-node/src/apiHelpers.js Outdated
Comment threadcreator-node/src/routes/tracks.js

@dmanjunathdmanjunath left a comment

Copy link
Copy Markdown
Contributor

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

Comment threadcreator-node/src/routes/tracks.js
Comment threadcreator-node/src/services/prometheusMonitoring/prometheusRegistry.js Outdated
Comment threadcreator-node/test/audiusUsers.test.js Outdated
@SidSethi
SidSethi marked this pull request as ready for review June 10, 2022 15:57

@joaquincasaresjoaquincasares left a comment

Copy link
Copy Markdown
Contributor

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!

Comment threadcreator-node/src/monitors/filesystem.js Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/prometheus.constants.js Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/prometheusRegistry.js Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/prometheus.constants.js Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/prometheus.constants.js Outdated
* syncHealthCheckController
*/
const syncHealthCheckController = async () => {
const syncHealthCheckController = async (req) => {

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
ContributorAuthor

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

@dmanjunathdmanjunath left a comment

Copy link
Copy Markdown
Contributor

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

Comment threadcreator-node/src/components/healthCheck/healthCheckController.js Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/README.md Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/README.md Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/README.md Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/README.md Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/README.md
Comment threadcreator-node/src/services/prometheusMonitoring/README.md Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/README.md Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/prometheus.constants.js Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/prometheus.constants.js Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/prometheus.constants.js Outdated

@joaquincasaresjoaquincasares left a comment

Copy link
Copy Markdown
Contributor

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! 🔥

Comment threadcreator-node/src/services/prometheusMonitoring/README.md Outdated
Comment threadcreator-node/src/services/prometheusMonitoring/README.md
Comment threadcreator-node/src/services/prometheusMonitoring/prometheus.constants.js Outdated
@SidSethi
SidSethi merged commit c29e7ae into masterJun 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-2334C-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
@AudiusProjectAudiusProject deleted a comment from linearBotSep 11, 2023
Sign up for freeto 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

@SidSethi@dmanjunath@joaquincasares