Skip to content

bitswap/httpnet: limit metric cardinality - #957

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

hsanjuan merged 8 commits into
mainfrom
httpnet-metric-cardinality

Conversation

@hsanjuan

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

codecov Bot commented Jun 19, 2025

Copy link
Copy Markdown

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

lidel commented Jun 23, 2025

Copy link
Copy Markdown
Contributor

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

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

Comment thread CHANGELOG.md Outdated
Comment thread CHANGELOG.md Outdated
@hsanjuan
hsanjuan enabled auto-merge June 27, 2025 08:35
@hsanjuan
hsanjuan merged commit 1f89f55 into main Jun 27, 2025
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