[receiver/prometheus] staleness markers aren't reported on failed scrapes after upgrade to 0.307.1

[dmitryax]

After upgrading Prometheus library to 0.307.1 in #43890 staleness markers aren't reported on failed scrapes. This causes some tests to fail. If it's expected the tests should be updated. If not, the issue should be pushed to prometheus project.

[github-actions]

Pinging code owners:

• receiver/prometheus: @Aneurysm9 @dashpole @ArthurSens @krajorama

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[krajorama]

Taking a look. Maybe related to prometheus/prometheus#16429

[krajorama]

It is related to that PR prometheus/prometheus#16429

The new code requires a cache entry for the series to be staleness tracked, however you only get a cache entry if the series has a storage reference other then 0. But we always return 0 storage reference from our appender (for both float and histogram samples).

[dashpole]

Should we be caching the storage references to fix this? Or do we need to fix it upstream?

[krajorama]

No idea, but I confirmed my theory, TestStaleNaNs passes with the diff below.

I don't know what the side effects would be and the diff doesn't handle native histograms. Also it's 9PM here, so I'm signing off. Maybe the original author of prometheus/prometheus#16429 could chime in what we should do. cc @prymitive

diff --git a/receiver/prometheusreceiver/internal/transaction.go b/receiver/prometheusreceiver/internal/transaction.go
index 1d68db9b7f..df1b03aaae 100644
--- a/receiver/prometheusreceiver/internal/transaction.go
+++ b/receiver/prometheusreceiver/internal/transaction.go
@@ -189,7 +189,7 @@ func (t *transaction) Append(_ storage.SeriesRef, ls labels.Labels, atMs int64,
                t.logger.Warn("failed to add datapoint", zap.Error(err), zap.String("metric_name", metricName), zap.Any("labels", ls))
        }
 
-       return 0, nil // never return errors, as that fails the whole scrape
+       return storage.SeriesRef(seriesRef), nil // never return errors, as that fails the whole scrape
 }
 
 // detectAndStoreNativeHistogramStaleness returns true if it detects
diff --git a/receiver/prometheusreceiver/metrics_receiver_non_numerical_test.go b/receiver/prometheusreceiver/metrics_receiver_non_numerical_test.go
index e646b8fd99..11564da606 100644
--- a/receiver/prometheusreceiver/metrics_receiver_non_numerical_test.go
+++ b/receiver/prometheusreceiver/metrics_receiver_non_numerical_test.go
@@ -46,7 +46,6 @@ var totalScrapes = 10
 
 // TestStaleNaNs validates that staleness marker gets generated when the timeseries is no longer present
 func TestStaleNaNs(t *testing.T) {
-       t.Skip("Skipping test until https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/43893 is resolved")
        var mockResponses []mockPrometheusResponse
        for i := range totalScrapes {
                if i%2 == 0 {

[krajorama]

The seriesRef generated by the receiver in my diff above is a hash of the label set that identifies the OTEL metric, not the Prometheus series, which means it would be the same for these series:

foo_bucket{}
foo_sum{}
foo_count{}

Which is not correct, since the scrape code would expect that each series has a different storage reference.

[krajorama]

On the other hand as far as I see the reference isn't used by the Prometheus scrape code, so we might as well return a value of 1 if the series was actually added. Making PR.

[krajorama]

Ready for review: #43925 .