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

fix: Use finite set of strings for HTTP metrics #4681

Merged
merged 5 commits into from
Apr 23, 2024

Conversation

jbencin
Copy link
Contributor

@jbencin jbencin commented Apr 13, 2024

Description

When sending metrics data to Prometheus, map URLs to a finite set of strings. This will reduce the number of unique strings the server has to keep track of. As an example, it might rewrite:

/v2/attachments/012c8195f4381d0b0eadc2daee6e2c5b8243f4ff

to

/v2/attachments/:hash

Applicable issues

Additional info (benefits, drawbacks, caveats)

NOTE: The test net::stackerdb::tests::sync::test_stackerdb_replica_2_neighbors_1_chunk is currently failing, but also fails in master, meaning it was not broken by this PR

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

Copy link

codecov bot commented Apr 16, 2024

Codecov Report

Attention: Patch coverage is 0% with 138 lines in your changes are missing coverage. Please review.

Project coverage is 8.20%. Comparing base (9b377f9) to head (234e2c1).

❗ Current head 234e2c1 differs from pull request most recent head dfb5f67. Consider uploading reports for the commit dfb5f67 to get more accurate results

Files Patch % Lines
stackslib/src/net/httpcore.rs 0.00% 32 Missing ⚠️
stackslib/src/net/rpc.rs 0.00% 14 Missing ⚠️
stackslib/src/monitoring/mod.rs 0.00% 5 Missing ⚠️
stackslib/src/net/api/callreadonly.rs 0.00% 3 Missing ⚠️
stackslib/src/net/api/getaccount.rs 0.00% 3 Missing ⚠️
stackslib/src/net/api/getattachment.rs 0.00% 3 Missing ⚠️
stackslib/src/net/api/getattachmentsinv.rs 0.00% 3 Missing ⚠️
stackslib/src/net/api/getblock.rs 0.00% 3 Missing ⚠️
stackslib/src/net/api/getconstantval.rs 0.00% 3 Missing ⚠️
stackslib/src/net/api/getcontractabi.rs 0.00% 3 Missing ⚠️
... and 22 more
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #4681       +/-   ##
===========================================
- Coverage   86.09%    8.20%   -77.89%     
===========================================
  Files         404      403        -1     
  Lines      293062   287578     -5484     
===========================================
- Hits       252312    23604   -228708     
- Misses      40750   263974   +223224     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jbencin
Copy link
Contributor Author

jbencin commented Apr 16, 2024

Added unit test for metrics_identifier(). This is now fully ready for review.

Also note that net::stackerdb::tests::sync::test_stackerdb_replica_2_neighbors_1_chunk is failing (at least locally for me), but also fails in the current master

@CharlieC3
Copy link
Member

I built and deployed this branch to one of our dev seed nodes. I can't comment on all endpoints addressed in this PR because it takes some time for all endpoints to eventually be hit, but it certainly seems like this will fix the issue!

Screenshot 2024-04-19 at 11 44 22 AM

CharlieC3
CharlieC3 previously approved these changes Apr 19, 2024
Copy link
Member

@CharlieC3 CharlieC3 left a comment

Choose a reason for hiding this comment

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

Very thorough! Thanks for much for taking the time to make these changes, it'll really help with our alerting and reducing false positives. Left a couple minor suggestions but otherwise LGTM :shipit:

stackslib/src/net/tests/httpcore.rs Outdated Show resolved Hide resolved
stackslib/src/net/tests/httpcore.rs Outdated Show resolved Hide resolved
@jbencin jbencin merged commit edef6f1 into stacks-network:master Apr 23, 2024
1 check passed
@jbencin jbencin deleted the fix/log-cardinality-all branch April 23, 2024 13:48
@CharlieC3
Copy link
Member

@wileyj What would it take to get a new version cut with this PR? It's not in 2.5.0.0.3. If a new version is coming soon, am fine with waiting.

@wileyj
Copy link
Contributor

wileyj commented May 6, 2024

@CharlieC3 nothing planned immediately, but we can start prepping something - i know there are some improvements to the signer that may be useful to release as well.
i would bring it up during eng. call and we can see what other PR's (if any) should be merged into an rc

@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

High cardinality for stacks_node_rpc_call_latencies_histogram metrics
5 participants