From c6793e717a78f5cf8b3d1b680b3ede6178e62ef2 Mon Sep 17 00:00:00 2001 From: Harsh Date: Wed, 8 Oct 2025 08:41:43 +0530 Subject: [PATCH 1/8] Remove obsolete check Signed-off-by: Harsh --- scrape/manager.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/scrape/manager.go b/scrape/manager.go index b4f198ecbf..3375031212 100644 --- a/scrape/manager.go +++ b/scrape/manager.go @@ -184,11 +184,6 @@ func (m *Manager) reload() { m.logger.Error("error reloading target set", "err", "invalid config id:"+setName) continue } - if scrapeConfig.ConvertClassicHistogramsToNHCBEnabled() && m.opts.EnableCreatedTimestampZeroIngestion { - // TODO(krajorama): fix https://github.com/prometheus/prometheus/issues/15137 - m.logger.Error("error reloading target set", "err", "cannot convert classic histograms to native histograms with custom buckets and ingest created timestamp zero samples at the same time due to https://github.com/prometheus/prometheus/issues/15137") - continue - } m.metrics.targetScrapePools.Inc() sp, err := newScrapePool(scrapeConfig, m.append, m.offsetSeed, m.logger.With("scrape_pool", setName), m.buffers, m.opts, m.metrics) if err != nil { From febd7341375fe70534057632a5afaa6ceb64dddb Mon Sep 17 00:00:00 2001 From: Harsh Date: Wed, 8 Oct 2025 09:07:24 +0530 Subject: [PATCH 2/8] test: Add TestNHCBAndCTZeroIngestion to verify simultaneous feature usage Signed-off-by: Harsh --- scrape/manager_test.go | 95 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/scrape/manager_test.go b/scrape/manager_test.go index 4e9feb7e17..0cde95e73c 100644 --- a/scrape/manager_test.go +++ b/scrape/manager_test.go @@ -1066,6 +1066,101 @@ func TestUnregisterMetrics(t *testing.T) { } } +// TestNHCBAndCTZeroIngestion verifies that both ConvertClassicHistogramsToNHCBEnabled +// and EnableCreatedTimestampZeroIngestion can be used simultaneously without errors. +// This test addresses issue #17216 by ensuring the previously blocking check has been removed. +func TestNHCBAndCTZeroIngestion(t *testing.T) { + t.Parallel() + + const mName = "test_histogram" + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Create a test histogram with created timestamp. + testHist := generateTestHistogram(0) + testHist.CreatedTimestamp = timestamppb.Now() + + app := &collectResultAppender{} + discoveryManager, scrapeManager := runManagers(t, ctx, &Options{ + EnableCreatedTimestampZeroIngestion: true, + EnableNativeHistogramsIngestion: true, + skipOffsetting: true, + }, &collectResultAppendable{app}) + defer scrapeManager.Stop() + + once := sync.Once{} + server := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + fail := true + once.Do(func() { + fail = false + w.Header().Set("Content-Type", `application/vnd.google.protobuf; proto=io.prometheus.client.MetricFamily; encoding=delimited`) + + ctrType := dto.MetricType_HISTOGRAM + w.Write(protoMarshalDelimited(t, &dto.MetricFamily{ + Name: proto.String(mName), + Type: &ctrType, + Metric: []*dto.Metric{{Histogram: testHist}}, + })) + }) + + if fail { + w.WriteHeader(http.StatusInternalServerError) + } + }), + ) + defer server.Close() + + serverURL, err := url.Parse(server.URL) + require.NoError(t, err) + + // Configuration with both convert_classic_histograms_to_nhcb enabled and CT zero ingestion enabled. + testConfig := fmt.Sprintf(` +global: + scrape_interval: 9999m + scrape_timeout: 5s + +scrape_configs: +- job_name: test + convert_classic_histograms_to_nhcb: true + static_configs: + - targets: ['%s'] +`, serverURL.Host) + + applyConfig(t, testConfig, scrapeManager, discoveryManager) + + // Wait for scrape to complete successfully. + ctx, cancel = context.WithTimeout(ctx, 1*time.Minute) + defer cancel() + require.NoError(t, runutil.Retry(100*time.Millisecond, ctx.Done(), func() error { + app.mtx.Lock() + defer app.mtx.Unlock() + + if len(app.resultHistograms) > 0 { + return nil + } + return errors.New("expected histogram samples, got none") + }), "after 1 minute") + + // Verify that samples were ingested (proving both features work together). + app.mtx.Lock() + defer app.mtx.Unlock() + + var got []histogramSample + for _, h := range app.resultHistograms { + if h.metric.Get(model.MetricNameLabel) == mName { + got = append(got, h) + } + } + + // With CT zero ingestion enabled and a created timestamp present, we expect 2 samples: + // one zero sample and one actual sample. + require.Len(t, got, 2, "expected 2 histogram samples (zero sample + actual sample)") + require.Equal(t, histogram.Histogram{}, *got[0].h, "first sample should be zero sample") + require.Equal(t, testHist.GetSampleSum(), got[1].h.Sum, "second sample should match input") +} + func applyConfig( t *testing.T, config string, From 3a7a8d75479c300e1e44b9b88d5f619402be3d54 Mon Sep 17 00:00:00 2001 From: Harsh Date: Sat, 11 Oct 2025 17:27:47 +0530 Subject: [PATCH 3/8] promql: Enhance TestNHCBAndCTZeroIngestion to validate exemplar parsing with created timestamps Signed-off-by: Harsh --- scrape/manager_test.go | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/scrape/manager_test.go b/scrape/manager_test.go index 0cde95e73c..fa76f95899 100644 --- a/scrape/manager_test.go +++ b/scrape/manager_test.go @@ -1069,6 +1069,8 @@ func TestUnregisterMetrics(t *testing.T) { // TestNHCBAndCTZeroIngestion verifies that both ConvertClassicHistogramsToNHCBEnabled // and EnableCreatedTimestampZeroIngestion can be used simultaneously without errors. // This test addresses issue #17216 by ensuring the previously blocking check has been removed. +// It also tests that exemplars are correctly parsed with both features enabled, addressing +// the original concern from issue #15137 about losing exemplars during CT parsing. func TestNHCBAndCTZeroIngestion(t *testing.T) { t.Parallel() @@ -1077,10 +1079,6 @@ func TestNHCBAndCTZeroIngestion(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - // Create a test histogram with created timestamp. - testHist := generateTestHistogram(0) - testHist.CreatedTimestamp = timestamppb.Now() - app := &collectResultAppender{} discoveryManager, scrapeManager := runManagers(t, ctx, &Options{ EnableCreatedTimestampZeroIngestion: true, @@ -1095,14 +1093,21 @@ func TestNHCBAndCTZeroIngestion(t *testing.T) { fail := true once.Do(func() { fail = false - w.Header().Set("Content-Type", `application/vnd.google.protobuf; proto=io.prometheus.client.MetricFamily; encoding=delimited`) + w.Header().Set("Content-Type", `application/openmetrics-text`) - ctrType := dto.MetricType_HISTOGRAM - w.Write(protoMarshalDelimited(t, &dto.MetricFamily{ - Name: proto.String(mName), - Type: &ctrType, - Metric: []*dto.Metric{{Histogram: testHist}}, - })) + // Expose a histogram with created timestamp and exemplars. + // This tests the fix for #15137 where exemplars were lost during CT parsing. + fmt.Fprint(w, `# HELP test_histogram A histogram with created timestamp and exemplars +# TYPE test_histogram histogram +test_histogram_bucket{le="0.0"} 1 +test_histogram_bucket{le="1.0"} 10 # {trace_id="trace-1"} 0.5 +test_histogram_bucket{le="2.0"} 20 # {trace_id="trace-2"} 1.5 +test_histogram_bucket{le="+Inf"} 30 # {trace_id="trace-3"} 2.5 +test_histogram_count 30 +test_histogram_sum 45.5 +test_histogram_created 1520430001 +# EOF +`) }) if fail { @@ -1158,7 +1163,12 @@ scrape_configs: // one zero sample and one actual sample. require.Len(t, got, 2, "expected 2 histogram samples (zero sample + actual sample)") require.Equal(t, histogram.Histogram{}, *got[0].h, "first sample should be zero sample") - require.Equal(t, testHist.GetSampleSum(), got[1].h.Sum, "second sample should match input") + require.NotEqual(t, 0.0, got[1].h.Sum, "second sample should have non-zero sum") + + // The test successfully completes, proving that both ConvertClassicHistogramsToNHCBEnabled + // and EnableCreatedTimestampZeroIngestion can work together without the error that was + // previously thrown. The original issue #15137 about losing exemplars during CT parsing + // in OpenMetrics format has been fixed independently in the parser layer. } func applyConfig( From a63414b8e8298ca93d87a5e845dcc0f4d4820f28 Mon Sep 17 00:00:00 2001 From: Harsh Date: Sun, 12 Oct 2025 21:29:18 +0530 Subject: [PATCH 4/8] suggested changes added Signed-off-by: Harsh --- scrape/manager_test.go | 60 ++++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 23 deletions(-) diff --git a/scrape/manager_test.go b/scrape/manager_test.go index fa76f95899..a7ea24d11d 100644 --- a/scrape/manager_test.go +++ b/scrape/manager_test.go @@ -1069,12 +1069,17 @@ func TestUnregisterMetrics(t *testing.T) { // TestNHCBAndCTZeroIngestion verifies that both ConvertClassicHistogramsToNHCBEnabled // and EnableCreatedTimestampZeroIngestion can be used simultaneously without errors. // This test addresses issue #17216 by ensuring the previously blocking check has been removed. -// It also tests that exemplars are correctly parsed with both features enabled, addressing -// the original concern from issue #15137 about losing exemplars during CT parsing. +// The test verifies that the presence of exemplars in the input does not cause errors, +// although exemplars are not preserved during NHCB conversion (as documented below). func TestNHCBAndCTZeroIngestion(t *testing.T) { t.Parallel() - const mName = "test_histogram" + const ( + mName = "test_histogram" + // The expected sum of the histogram, as defined by the test's OpenMetrics exposition data. + // This value (45.5) is the sum reported in the test_histogram_sum metric below. + expectedHistogramSum = 45.5 + ) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -1095,8 +1100,7 @@ func TestNHCBAndCTZeroIngestion(t *testing.T) { fail = false w.Header().Set("Content-Type", `application/openmetrics-text`) - // Expose a histogram with created timestamp and exemplars. - // This tests the fix for #15137 where exemplars were lost during CT parsing. + // Expose a histogram with created timestamp and exemplars to verify no parsing errors occur. fmt.Fprint(w, `# HELP test_histogram A histogram with created timestamp and exemplars # TYPE test_histogram histogram test_histogram_bucket{le="0.0"} 1 @@ -1123,6 +1127,7 @@ test_histogram_created 1520430001 // Configuration with both convert_classic_histograms_to_nhcb enabled and CT zero ingestion enabled. testConfig := fmt.Sprintf(` global: + # Use a very long scrape_interval to prevent automatic scraping during the test. scrape_interval: 9999m scrape_timeout: 5s @@ -1135,35 +1140,44 @@ scrape_configs: applyConfig(t, testConfig, scrapeManager, discoveryManager) - // Wait for scrape to complete successfully. - ctx, cancel = context.WithTimeout(ctx, 1*time.Minute) - defer cancel() - require.NoError(t, runutil.Retry(100*time.Millisecond, ctx.Done(), func() error { + // Verify that the scrape pool was created (proves the blocking check was removed). + require.Eventually(t, func() bool { + scrapeManager.mtxScrape.Lock() + defer scrapeManager.mtxScrape.Unlock() + _, exists := scrapeManager.scrapePools["test"] + return exists + }, 5*time.Second, 100*time.Millisecond, "scrape pool should be created for job 'test'") + + // Helper function to get matching histograms to avoid race conditions. + getMatchingHistograms := func() []histogramSample { app.mtx.Lock() defer app.mtx.Unlock() - if len(app.resultHistograms) > 0 { - return nil + var got []histogramSample + for _, h := range app.resultHistograms { + if h.metric.Get(model.MetricNameLabel) == mName { + got = append(got, h) + } } - return errors.New("expected histogram samples, got none") - }), "after 1 minute") + return got + } + + require.Eventually(t, func() bool { + return len(getMatchingHistograms()) > 0 + }, 1*time.Minute, 100*time.Millisecond, "expected histogram samples, got none") // Verify that samples were ingested (proving both features work together). - app.mtx.Lock() - defer app.mtx.Unlock() - - var got []histogramSample - for _, h := range app.resultHistograms { - if h.metric.Get(model.MetricNameLabel) == mName { - got = append(got, h) - } - } + got := getMatchingHistograms() // With CT zero ingestion enabled and a created timestamp present, we expect 2 samples: // one zero sample and one actual sample. require.Len(t, got, 2, "expected 2 histogram samples (zero sample + actual sample)") require.Equal(t, histogram.Histogram{}, *got[0].h, "first sample should be zero sample") - require.NotEqual(t, 0.0, got[1].h.Sum, "second sample should have non-zero sum") + require.InDelta(t, expectedHistogramSum, got[1].h.Sum, 1e-9, "second sample should retain the expected sum") + + // Note: Exemplars from classic histogram buckets are not preserved when converting to NHCB + // because the bucket series are replaced with a single native histogram sample. This is expected + // behavior. The test verifies that the presence of exemplars in the input doesn't cause errors. // The test successfully completes, proving that both ConvertClassicHistogramsToNHCBEnabled // and EnableCreatedTimestampZeroIngestion can work together without the error that was From a1e163a462dcb7db2e342e7a45a6ffbf007eea49 Mon Sep 17 00:00:00 2001 From: harsh kumar <135993950+hxrshxz@users.noreply.github.com> Date: Mon, 13 Oct 2025 00:50:42 +0530 Subject: [PATCH 5/8] Update scrape/manager_test.go Co-authored-by: George Krajcsovits Signed-off-by: harsh kumar <135993950+hxrshxz@users.noreply.github.com> --- scrape/manager_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scrape/manager_test.go b/scrape/manager_test.go index a7ea24d11d..f4dda1b5b8 100644 --- a/scrape/manager_test.go +++ b/scrape/manager_test.go @@ -1104,8 +1104,8 @@ func TestNHCBAndCTZeroIngestion(t *testing.T) { fmt.Fprint(w, `# HELP test_histogram A histogram with created timestamp and exemplars # TYPE test_histogram histogram test_histogram_bucket{le="0.0"} 1 -test_histogram_bucket{le="1.0"} 10 # {trace_id="trace-1"} 0.5 -test_histogram_bucket{le="2.0"} 20 # {trace_id="trace-2"} 1.5 +test_histogram_bucket{le="1.0"} 10 # {trace_id="trace-1"} 0.5 123456789 +test_histogram_bucket{le="2.0"} 20 # {trace_id="trace-2"} 1.5 123456780 test_histogram_bucket{le="+Inf"} 30 # {trace_id="trace-3"} 2.5 test_histogram_count 30 test_histogram_sum 45.5 From 5244b0e3f778512406166f8946a80eeeef823e01 Mon Sep 17 00:00:00 2001 From: harsh kumar <135993950+hxrshxz@users.noreply.github.com> Date: Mon, 13 Oct 2025 00:50:58 +0530 Subject: [PATCH 6/8] Update scrape/manager_test.go Co-authored-by: George Krajcsovits Signed-off-by: harsh kumar <135993950+hxrshxz@users.noreply.github.com> --- scrape/manager_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/scrape/manager_test.go b/scrape/manager_test.go index f4dda1b5b8..0ee1549eb3 100644 --- a/scrape/manager_test.go +++ b/scrape/manager_test.go @@ -1175,9 +1175,7 @@ scrape_configs: require.Equal(t, histogram.Histogram{}, *got[0].h, "first sample should be zero sample") require.InDelta(t, expectedHistogramSum, got[1].h.Sum, 1e-9, "second sample should retain the expected sum") - // Note: Exemplars from classic histogram buckets are not preserved when converting to NHCB - // because the bucket series are replaced with a single native histogram sample. This is expected - // behavior. The test verifies that the presence of exemplars in the input doesn't cause errors. + require.Len(t, app.resultExemplars, 2, "expected 2 exemplars from histogram buckets") // The test successfully completes, proving that both ConvertClassicHistogramsToNHCBEnabled // and EnableCreatedTimestampZeroIngestion can work together without the error that was From a612cd8954cd7b7235b36a5609ae6d4350f5f7cd Mon Sep 17 00:00:00 2001 From: harsh kumar <135993950+hxrshxz@users.noreply.github.com> Date: Mon, 13 Oct 2025 12:41:46 +0530 Subject: [PATCH 7/8] Update scrape/manager_test.go Co-authored-by: George Krajcsovits Signed-off-by: harsh kumar <135993950+hxrshxz@users.noreply.github.com> --- scrape/manager_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/scrape/manager_test.go b/scrape/manager_test.go index 0ee1549eb3..e2786a2157 100644 --- a/scrape/manager_test.go +++ b/scrape/manager_test.go @@ -1177,10 +1177,6 @@ scrape_configs: require.Len(t, app.resultExemplars, 2, "expected 2 exemplars from histogram buckets") - // The test successfully completes, proving that both ConvertClassicHistogramsToNHCBEnabled - // and EnableCreatedTimestampZeroIngestion can work together without the error that was - // previously thrown. The original issue #15137 about losing exemplars during CT parsing - // in OpenMetrics format has been fixed independently in the parser layer. } func applyConfig( From 4d7d8ebcfac172b060b0a46c8ccfab6c74391a6f Mon Sep 17 00:00:00 2001 From: Harsh Date: Mon, 13 Oct 2025 13:59:10 +0530 Subject: [PATCH 8/8] lint fixes Signed-off-by: Harsh --- scrape/manager_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/scrape/manager_test.go b/scrape/manager_test.go index e2786a2157..483472379c 100644 --- a/scrape/manager_test.go +++ b/scrape/manager_test.go @@ -1174,9 +1174,7 @@ scrape_configs: require.Len(t, got, 2, "expected 2 histogram samples (zero sample + actual sample)") require.Equal(t, histogram.Histogram{}, *got[0].h, "first sample should be zero sample") require.InDelta(t, expectedHistogramSum, got[1].h.Sum, 1e-9, "second sample should retain the expected sum") - require.Len(t, app.resultExemplars, 2, "expected 2 exemplars from histogram buckets") - } func applyConfig(