Skip to content

bitswap/httpnet: limit metric cardinality#957

Merged
hsanjuan merged 8 commits intomainfrom
httpnet-metric-cardinality
Jun 27, 2025
Merged

bitswap/httpnet: limit metric cardinality#957
hsanjuan merged 8 commits intomainfrom
httpnet-metric-cardinality

Conversation

@hsanjuan
Copy link
Contributor

Currently, we label the StatusCounter metric with the hostname of the http endpoint. If there was a high number of endpoints, this would cause high cardinality for this metric.

This is a non-problem now as there are very few HTTP endpoints. This commit addresses it anyways by only labelling the metric hostnames when the Allowlist is enabled, therefore limiting labels to hostnames in the allowlist.

Currently, we label the StatusCounter metric with the hostname of the http
endpoint. If there was a high number of endpoints, this would cause high
cardinality for this metric.

This is a non-problem now as there are very few HTTP endpoints. This commit
addresses it anyways by only labelling the metric hostnames when the Allowlist
is enabled, therefore limiting labels to hostnames in the allowlist.
@hsanjuan hsanjuan self-assigned this Jun 19, 2025
@hsanjuan hsanjuan requested a review from a team as a code owner June 19, 2025 07:46
@codecov
Copy link

codecov bot commented Jun 19, 2025

Codecov Report

Attention: Patch coverage is 59.09091% with 9 lines in your changes missing coverage. Please review.

Project coverage is 61.56%. Comparing base (8abf4db) to head (4f92c60).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
bitswap/network/httpnet/httpnet.go 14.28% 6 Missing ⚠️
bitswap/network/httpnet/metrics.go 80.00% 2 Missing and 1 partial ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #957      +/-   ##
==========================================
- Coverage   61.62%   61.56%   -0.07%     
==========================================
  Files         254      254              
  Lines       31409    31425      +16     
==========================================
- Hits        19357    19346      -11     
- Misses      10480    10502      +22     
- Partials     1572     1577       +5     
Files with missing lines Coverage Δ
bitswap/network/httpnet/metrics.go 92.30% <80.00%> (-4.06%) ⬇️
bitswap/network/httpnet/httpnet.go 64.83% <14.28%> (-0.81%) ⬇️

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hsanjuan
Copy link
Contributor Author

I don't know why I'm feeling that I should go the extra mile and have an option to log all endpoints.

@lidel
Copy link
Member

lidel commented Jun 23, 2025

@hsanjuan perhaps a compromise is to have "HTTPRetrieval.Metriclist" in Kubo, and use it when HTTPRetrieval.Allowlist is not defined? This way we could track specific provider without limiting endpoints and without balooning cardinality globally.

Logging all endpoints may be used in staging/testing, we could add * (wildcard) support as explicit opt-in that one can set to enable metrics for all endpoints + log warning if someone has it set, so its not set in production by mistake.

@lidel lidel mentioned this pull request Jun 23, 2025
46 tasks
@hsanjuan hsanjuan requested a review from guillaumemichel June 25, 2025 14:30
@hsanjuan
Copy link
Contributor Author

I have added an option for this setting. It can be bubbled later to Kubo/Rainbow when merged.

@hsanjuan hsanjuan enabled auto-merge June 27, 2025 08:35
@hsanjuan hsanjuan merged commit 1f89f55 into main Jun 27, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants