Skip to content

fix(config): disable default Prometheus telemetry sink (CON-335) - #4032

Open
amir-deris wants to merge 1 commit into
mainfrom
amir/CON-335-telemetry-sink-active-issue
Open

fix(config): disable default Prometheus telemetry sink (CON-335)#4032
amir-deris wants to merge 1 commit into
mainfrom
amir/CON-335-telemetry-sink-active-issue

Conversation

@amir-deris

@amir-deris amir-deris commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Describe your changes and provide context

CON-335

Default app.toml had telemetry.enabled = true and prometheus-retention-time = 7200. That activates the Prometheus metrics sink in telemetry.New() even when Tendermint's [instrumentation].prometheus is disabled and nothing is scraping application metrics. Metrics accumulate in the sink for the full retention window without being collected.

This PR changes the default prometheus-retention-time from 7200 to 0. Freshly initialized nodes still get the bounded in-memory telemetry sink (useful for SIGUSR1 dumps), but the Prometheus sink is only created when an operator explicitly sets a positive retention value.

Existing nodes with prometheus-retention-time already written in their app.toml are unchanged. Explicit configs (e.g. docker/rpcnode/config/app.toml) are left as-is.

Testing performed to validate your change

  • go test ./sei-cosmos/server/config/
  • go test ./sei-cosmos/telemetry/
  • go test ./config/cosmosbase/
  • Added TestMetrics_PromDisabledByDefaultRetention to assert retention 0 does not enable the Prometheus sink
  • Updated config characterization golden files and divergence records for the new default

Default prometheus-retention-time to 0 so freshly initialized nodes keep
the bounded in-memory sink without starting the Prometheus sink until an
operator sets a positive retention value.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedAug 27, 2026, 11:33 AM

@amir-deris
amir-deris requested a review from masih August 27, 2026 11:34
@amir-deris
amir-deris marked this pull request as ready for review August 27, 2026 11:34
@cursor

cursor Bot commented Aug 27, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Default-only change for new seid init configs; existing app.toml values and explicit operator overrides are unchanged. Low operational risk aside from operators who relied on implicit 7200s Prometheus retention on fresh nodes.

Overview
Changes the default telemetry.prometheus-retention-time from 7200 to 0 in DefaultConfig(), so newly generated app.toml no longer turns on the Prometheus metrics sink by default.

Telemetry stays enabled = true, so the bounded in-memory sink (e.g. SIGUSR1 dumps) still works. telemetry.New only creates the Prometheus sink when retention is positive; zero leaves prometheusEnabled false and avoids holding app metrics for hours when nothing scrapes them.

telemetry.prometheus-retention-time is no longer listed as a divergence between generated config and absent-key resolution (both sides are 0). Tests, the server config golden, and characterization tables are updated; TestMetrics_PromDisabledByDefaultRetention asserts retention 0 does not enable Prometheus.

Reviewed by Cursor Bugbot for commit 559e4b3. Bugbot is set up for automated code reviews on this repo. Configure here.

@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.21%. Comparing base (4820c92) to head (559e4b3).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4032      +/-   ##
==========================================
- Coverage   61.21%   60.21%   -1.01%     
==========================================
  Files        2153     2054      -99     
  Lines      188381   176696   -11685     
==========================================
- Hits       115324   106401    -8923     
+ Misses      62315    60520    -1795     
+ Partials    10742     9775     -967     
Flag Coverage Δ
sei-chain-pr 88.77% <100.00%> (?)
sei-db 69.80% <ø> (ø)
sei-db-state-db ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
sei-cosmos/server/config/config.go 97.80% <100.00%> (ø)

... and 99 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The default prometheus-retention-time drops to 0 and the characterization records (agreement map, divergence table, fuzz row rationale, golden) are updated consistently with that. However the change only reaches the seid init pipeline: initAppConfig still hard-sets 60, so the second app.toml-generation path keeps enabling the Prometheus sink, and the PR's stated behavior does not hold there.

Findings: 1 blocking | 2 non-blocking | 1 posted inline

Blockers

  • None at the file/PR level.
  • 1 blocking issue(s) flagged inline on specific lines.

Non-blocking

  • [suggestion] config/cosmosbase/cosmosbase.go:54-56 is now factually wrong: the forMode godoc says the self-written file pipeline writes "a metric retention of sixty where this writes seven thousand two hundred". After this change seid init writes 0, not 7200. That paragraph is the package's record of how the two generation pipelines differ, so it should be corrected in the same PR (and, given the divergence is now qualitative — one path enables the Prometheus sink and the other does not — it is worth pinning with a test rather than prose).
  • [suggestion] Operator-visible consequence not covered anywhere in the PR: with retention 0 and telemetry.enabled = true, Metrics.gatherPrometheus returns prometheus metrics are not enabled, so the app API server's GET /metrics?format=prometheus (sei-cosmos/server/api/server.go:155) starts failing for any freshly seid init-ed node that previously scraped it. Worth a release note / upgrade-guide line so operators know to set a positive retention explicitly.

Telemetry: telemetry.Config{
Enabled: true,
PrometheusRetentionTime: 7200,
PrometheusRetentionTime: 0,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[blocker] This only changes one of the two app.toml-generation pipelines. cmd/seid/cmd/root.go:421 still sets srvCfg.Telemetry.PrometheusRetentionTime = 60 on the config returned by initAppConfig, and that config is what PersistentPreRunEmgr.ApplyInterceptConfigsPreRunHandler (sei-cosmos/server/util.go:296-313) writes whenever app.toml is missing for any command other than init. So a node whose app.toml is auto-created by seid start (or any other non-init subcommand) still gets prometheus-retention-time = 60 and still starts the Prometheus sink with no operator opt-in — the PR description's claim that "the Prometheus sink is only created when an operator explicitly sets a positive retention value" does not hold for that path.

Before this change both pipelines enabled the sink (7200 vs 60) and only the retention window differed; now they disagree about whether the sink exists at all, which is a new and silent inconsistency. Either drop the = 60 override in root.go too, or state explicitly why the self-written file should keep Prometheus on.

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.

Worth taking a look at this one Amir. This is one of the challenges of existing config generation. It's done in multiple places. Ideally we won't need to deal with this in the near future

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.

4 participants