receiver/prometheus: use actual interval startTimeMs for cumulative types

[odeke-em]

With this change, we now infer the actual interval startTime
for cumulative types from the original starttime interval reinforcing
what the OpenTelemetry Proto recommendations say in
https://github.com/open-telemetry/opentelemetry-proto/blob/bc8ee79d8e01faf3310af2987268e94285f354da/opentelemetry/proto/metrics/v1/metrics.proto#L132-L140

Fixes #3691

[bogdandrutu]

Please rebase, this PR contains changes already merged

[odeke-em]

Please rebase, this PR contains changes already merged

Rebase done, please take a look @bogdandrutu!

[bogdandrutu]

Please review @Aneurysm9 @rakyll @dashpole @punya

[dashpole]

It looks like an Appender is created when the config is loaded: https://github.com/prometheus/prometheus/blob/5cc30b716e23c98863093c0ed39315c1fe7c103e/scrape/manager.go#L257. That would mean that this start time is the start time of the first metric that the collector sees, which is approximately the start time of the collector. That seems like it would be incorrect in many cases. Is there something i'm missing?

[odeke-em]

Nope, for every Scrape, a new storage.Appender is requested, not when the collector is started. This means that
even a new transaction is created per scrape and at the end either .Commit() or .Rollback() is invoked. This isn't a problem.

[dashpole]

Got it, thanks. Is the storage.Appender created after the scrape finishes, or before the scrape is started? We just need to make sure the start time isn't before the metric's timestamp

[bogdandrutu]

Another option for the moment is to leave the starttime unset (0) which is conforming with our specs, then fix this the correct timestamp later. This is to unblock the progress.

[odeke-em]

Got it, thanks. Is the storage.Appender created after the scrape finishes, or before the scrape is started? We just need to make sure the start time isn't before the metric's timestamp

@dashpole we infer that startTime for the very first time that's passed into (*transaction).Append so it will ALWAYS be the first timestamp. initTransaction is lazily loaded and .startTimeMs will ALWAYS be set after the first .Append.

opentelemetry-collector/receiver/prometheusreceiver/internal/transaction.go

Lines 116 to 118 in f804c9c

	if tr.startTimeMs < 0 {
	tr.startTimeMs = t
	}

opentelemetry-collector/receiver/prometheusreceiver/internal/transaction.go

Lines 136 to 140 in f804c9c

	if tr.isNew {
	if err := tr.initTransaction(ls); err != nil {
	return 0, err
	}
	}

@dashpole to answer your question about Prometheus (but it is irrelevant to this code) the storage.Appender is created before the scrape begins per https://github.com/prometheus/prometheus/blob/1a430e5f894d6575478bcef52d25bfbe7134947e/scrape/scrape.go#L1182-L1237 or visually

Another option for the moment is to leave the starttime unset (0) which is conforming with our specs, then fix this the correct timestamp later. This is to unblock the progress.

@bogdandrutu thanks for raising this, I didn't know about using 0 conforming with specs. No, we are already good, the intervalStartTime is inferred from the very first start time when (*transaction).Append so we are all set.

[odeke-em]

Thank you for the review @dashpole and @bogdandrutu!