If we are open to adding "internal" fields to protocols, we could consider adding labels directly to the writev2.Timeseries... I prototyped that, and it reduces allocations by another 20%.

I'm more seriously considering not translating to PRW at all, and writing straight to the appender... Seems like it is causing way more problems than it solves, and I don't love adding fields to protocols that are meant to be used externally

I think I might have figured out why there is such a high performance cost. isSameMetric has a bug where it returns true as long as the labels are the same length.

This is comparing elements in ts.Labels to themselves, rather than to the input labels.

FYI added prometheus/common#801

@krajorama are we sure resurrecting dead PRW1 is a good idea? What's blocking on PRW2?

#16784 (review)

I would say this code has to go through PRW2 path or natively, there's little point in sticking to the old path forever

@krajorama are we sure resurrecting dead PRW1 is a good idea? What's blocking on PRW2?

#16784 (review)

I don't want to resurrect PRW1.

However I don't want to see +35% CPU in the OTLP endpoint.

I'm working on a prototype to not use either PRW1 nor PRW2 in the OTLP endpoint, rather call the storage.Appender interface directly instead.

Yes, the native way is the long term solution, make sense

isSameMetric has a bug

Bug was fixed in open-telemetry/opentelemetry-collector-contrib#35763 ?

Did we set up a mechanism to be informed of bugfixes in these two copies of the code?

Did a little thought experiment, to see what it would take to use the Appender interface: #16827

Doesn't look terrible at first glance. Certainly if the Appender is writing directly to TSDB, I can imagine it being pretty fast.

In Mimir we don't write to TSDB directly, we use PRW1.0 for further processing and sending over to storage. Which means we'd need to take the input to the Appender interface and convert to PRW 1.0 . But the Appender in Mimir takes stringlabels and PRW1 has list of labels. So we'd be converting labels to stringlabels for the Appender and then back to a list of pairs of strings for PRW1.0. That doesn't sound great.

Also the Appender interface allows for random access, that is I could append a float sample to series "A", write a 1000 different series then write the exemplar for series "A". In fact adding the exemplars for classic histograms does this in the code, writes the buckets first, then the exemplars for those buckets. This random access would be insane to implement in an Appender that just spits out PRW1.

So at second glance I'm not convinced this is the right direction long term.

the Appender in Mimir takes stringlabels

When I was trying to do something comparable, I extended Appender with a method taking a callback, so I could fetch the SeriesRef without having to construct a new labels.Labels except for brand-new series.

It was in this (unmerged) PR: grafana/mimir#6979

	GetRefFunc(hash uint64, cmp func(labels.Labels) bool) (SeriesRef, labels.Labels)

So at second glance I'm not convinced this is the right direction long term.

But looking at the two converters side by side, there is layer we could make into an interface. It would take map[string]string for labels and take sample+metadata+exemplar(s) in one go. That translates well for TSDB Appender or converting into some protocol I would think.

So maybe we could do that if PRW2.0 doesn't work out for this use case?

It would take map[string]string for labels

Prometheus stopped using maps for labels internally in 2016, as it is far cheaper to use a slice.

It would take map[string]string for labels

Prometheus stopped using maps for labels internally in 2016, as it is far cheaper to use a slice.

The reason to suggest it is because the conversion code uses it already :

Could be something else as long as we don't need to unpack it for either branch: Appender nor Remote Write

the conversion code uses it already

That's pretty internal to that function, apparently aimed at detecting duplicates. Though this is questionable - labels need to be sorted anyway so deferring that detection to after you've finished and sorted them will be much cheaper.

Incidentally I couldn't immediately see how labels were sorted in that code; it happens as a side-effect of timeSeriesSignature(). Which should at least be commented.

I've also been working on a migration PR (instead of a re-write): main...dashpole:prometheus:otlp_to_labels. I haven't updated the unit tests in the translator package, but it passes the tests in write_test.go.

Also the Appender interface allows for random access, that is I could append a float sample to series "A", write a 1000 different series then write the exemplar for series "A". In fact adding the exemplars for classic histograms does this in the code, writes the buckets first, then the exemplars for those buckets. This random access would be insane to implement in an Appender that just spits out PRW1.

Yeah, that is pretty much what the OTel prometheusreceiver has to do, and it is pretty complex.

Thanks for discussions and experiments!

Just to summarize the current discussion (let me know if I'm wrong)

1. Prometheus has:

• PRW1 -> Appender
• PRW2 -> Appender
• Scrape -> Appender
• OTLP -> PRW1 -> Appender # Causes complexities, ideally it's OTLP -> Appender.

2. Moving to Appender looks like the best "native" option, however it's not ideal for Mimir, because Mimir does depend on OTLP to PRW1 mechanism because:

• PRW1 -> Mimir
• PRW2 -> PRW1 -> Mimir
• OTLP -> PRW1 -> Mimir # Implementing OTLP -> Appender will not work with Mimir and OTLP -> Appender -> PRW is tricky to non sequential appends.

3. Also Otel collector uses Prometheus appender interface in some paths and have same issue on appender:

• (prometheusreceiver) Scrape -> Appender -> OTel data model # Also tricky due to non sequential appends.
• (prometheusremotewritereceiver) PRW 1/2 -> Otel data model

Unblocking questions then...

A) It feels it's still nice to purse OTLP -> Appender long term, how we could unblock this? Would it be helpful for Mimir to vendor OTLP -> PRW1 path mid-term to unblock Prometheus upstream?
B) Should we create appender API (v2?) for "atomic" appends (series + float + ts + exemplar + type and unit + CT appends). Am I right that for all of ingestion formats (other than experimental metadata on PRW1 and metadata-wal-records feature) already would use this? -- scrape, PRW and OTLP.

Funny enough, related to (B) I already "had" to do something like this to support efficient CTs appends...

Also if we do (B) how breaking we want it to be... do we change in-place or we make Thanos, Cortex, Mimir life easier by maintaining both v1 and v2 paths for key flows...

@krajorama @bboreham @dashpole

Yes, there's always the possibility to deviate our Mimir fork more for some time.

I'd like to do a POC, where our Appender that converts to RW1 would return the index+1 of the time series in the slice of time series in the RW1 request as reference. Of course 0 means this is a new time series and we append.

I'm guessing that would solve the issue of non atomic Append calls.

I have to think about the stringlabels and probably discuss with @bboreham . I could imagine a trick where we fake things by putting the stringlabel data into a special label: __stringlabels__ and also set some header or something to let the other side know how to unmarshal it.

I think that makes sense. It actually cleans up the translator code to do that too. I prototyped it in 97ae3e1#diff-3d1ac744df30ff13a5bc3f2fe4dd9a735714cdda480b0f9520e05be97ec3bc8aR37

Closing in favor of #16951