fix(byoo-otel-collector): set honor_timestamps false on federated scrape jobs - #1530
Open
kristinapathak wants to merge 1 commit into
Open
fix(byoo-otel-collector): set honor_timestamps false on federated scrape jobs#1530kristinapathak wants to merge 1 commit into
kristinapathak wants to merge 1 commit into
Conversation
…ape jobs The collector federates from an in-cluster Prometheus that may run several replicas behind a single service. Each replica keeps its own scrape clock, and the rendered labelkeep strips prometheus_replica, so consecutive samples of one series can carry timestamps that move backwards. A Prometheus-compatible receiver rejects the entire write request in that case, dropping unrelated metrics batched with it. Setting honor_timestamps: false makes the collector stamp federated samples with its own monotonic scrape time, which neutralizes out-of-order, duplicate-for-timestamp, and out-of-bounds for the federated path regardless of how the upstream Prometheus is deployed. The tradeoff is up to one scrape interval (30s) of timestamp imprecision, already accepted for the kubernetes-cadvisor job. Applied to every federated job that lacked it in the k8s templates of both renderers. The vm templates scrape their targets directly, and direct scrape targets expose no explicit timestamps, so they are unchanged. Also adds the missing go-lib requirement to tools/byoo so the go-lib example generator builds. It was replace'd but never require'd. Closes #1529 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (86)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughThe change sets ChangesFederated scrape configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Kubernetes federated scrapes now assign collection-time timestamps, preventing replica clock skew from causing rejected metric writes while retaining the accepted scrape-interval timestamp imprecision. Current templates, generated examples, and render coverage align with this behavior, with no remaining merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Out of Scope Changes checkExplanation All changes support the linked issue: renderer templates, generated configuration examples, rendered-config tests, and the dependency required for example generation. No unrelated code changes are evident. Full details: Docstring CoverageExplanation Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (84 skipped: 84 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 golangci-lint (2.13.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: -mod may only be set to readonly or vendor when in workspace mode, but it is set to "mod"\n\tRemove the -mod flag to use the default readonly value, \n\tor set GOWORK=off to disable workspace mode.\n" Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The BYOO collector scrapes an in-cluster Prometheus through
/federate. ThatPrometheus commonly runs several replicas behind a single service, and we do not
control how it is deployed.
When it does, four things combine:
the service endpoints.
different timestamps on each replica.
/federatereturns explicit timestamps, and the collector honors them bydefault (
honor_timestampsdefaults totrue).labelkeepthat stripsprometheus_replica, so nothing downstream distinguishes the two sources.Consecutive samples of one series can therefore carry timestamps that move
backwards. A Prometheus-compatible receiver treats that as a conflict
(out-of-order, duplicate-for-timestamp, or out-of-bounds) and rejects the entire
write request rather than the offending series, so unrelated metrics batched
with it are dropped too.
honor_timestamps: falsemakes the collector stamp federated samples with itsown scrape time. Its clock is monotonic, so this neutralizes all three conflict
classes for the federated path regardless of how the upstream Prometheus is
deployed. The tradeoff is up to one scrape interval (30s) of timestamp
imprecision, already accepted in this codebase for the
kubernetes-cadvisorjob.
What changed
honor_timestamps: falseon every federated scrape job that lacked it, in thek8s templates of both renderers:
src/compute-plane-services/byoo-otel-collector/internal/otelconfig/source_templates/src-config-k8s-{container,helm}.yaml.tmpl,with
generator/gen/andinternal/otelconfig/templates/regenerated viamake update-config-templatesrc/libraries/go/lib/pkg/otelconfig/backendconfig/templates/config-k8s-{container,helm}.yaml.tmplkube-state-metricsandnvidia-dcgm-exporter. Thecurrent renderer's k8s templates also federate
nvcf-workerandnvca, andthose jobs were missing the setting too. They hit the same service and are
affected identically, so they are included.
kubernetes-cadvisoralready hadit. Four jobs per template in the current renderer, two per template in the
legacy one, which has no
nvcf-workerornvcajob.whose
metrics_pathis/federatemust sethonor_timestamps: false. Thetest is written against the rendered config, not the template text, and also
asserts the
vmtemplates produce no federated jobs, which pins the scopingdecision below.
byoo-otel-collector/examples/plus the metric-subset golden, and 36 in
go-lib/examples/. Every diff isexactly the added
honor_timestampslines; novmexample changed.tools/byoo: added the missinggo-librequirement so the go-lib examplegenerator builds. It was
replaced but neverrequired.The
opentelemetry-collectorself-scrape job is untouched.Why the
vmtemplates are excludedThe
vmtemplates do not federate. They scrapenvidia-dcgm-exporter,kube-state-metricsandnvcf-workerat/metrics, and the kubelet cadvisorendpoint through a
__metrics_path__relabel. Direct scrape targets expose noexplicit timestamps, so
honor_timestampshas nothing to override and settingit is a no-op. Verified by reading all four
vmtemplates in both renderers: no/federatepath appears in any of them. The new tests assert this holds.Customer Release Notes
Platform metrics federated from the in-cluster Prometheus are now timestamped at
collection time by the BYOO collector rather than carrying the upstream scrape
timestamp. This makes ingest resilient to clock differences between upstream
Prometheus replicas, at the cost of up to one scrape interval (30s) of timestamp
imprecision.
Plan Summary
Not applicable.
Usage
Not applicable.
Testing
byoo-otel-collector:GOWORK=off go test ./...passes.go-lib:GOWORK=off GOFLAGS=-mod=vendor go test ./pkg/otelconfig/...passes.Red output named exactly the jobs missing the setting:
nvcf-worker,nvca,kube-state-metrics,nvidia-dcgm-exporterin the current renderer, andkube-state-metrics,nvidia-dcgm-exporterin the legacy one.overwritten. Across all 77 regenerated examples the only changed line in the
whole diff is
+ honor_timestamps: false. The commit is 431insertions and 0 deletions.
gofmtandgo vetclean on both changed test files.Pre-existing failures unrelated to this change:
TestRunandTestRun_sidecar_deploymentsinsrc/libraries/go/lib/cmd/icms-translate, andmake check-testdatafailing on BSDsed.Notes
This is not confirmed to be the cause of any specific ingest failure. The
receive-side logs that would name the conflicting series are not available to
us. The change is justified on its own merits: it makes the federated path
immune to upstream replica clock skew, which we cannot control.
Regenerating the go-lib examples surfaced an environment dependency worth
flagging.
render.goemitsca_fileonly when the file exists on disk at thesecrets path, so a naive regeneration on a host without it silently drops that
line from four
kratos_thanos_stgexamples. The regeneration here supplied thefixture so the committed files stay byte-accurate, but the generator is not
hermetic and a future contributor will hit this.
The four
*_validator.yamlexamples underbyoo-otel-collector/examples/arestale: their input
testdata/validator.jsonno longer exists, somake update-examplesdoes not regenerate them and they still reflect an oldertemplate shape. Left alone as out of scope.
References
Closes #1529
Related Pull Requests
target_infoon the remote-write exporter. The two are independent hypothesesabout what causes write requests to be rejected; neither depends on the other
and either can land first.
Dependencies
No new third-party packages. The
tools/byoogo.mod/go.sumchange adds arequirement on the in-repo
go-libmodule, which was already wired in via areplacedirective. This overlaps with the same fix in #1527; whichever mergessecond will need the overlap resolved.
🤖 Generated with Claude Code
Summary by CodeRabbit