Just re-noticed this discussions, thanks for helping Mirco! Answering in place, given some updates recently.
> As mentioned above, this would be also a good opportunity to cleanup the proto generation code using buf. We can move to *buf* straight away, even now in all places. It works with or without gogoproto option fields in the proto format. No need to change proto format (although it would be cleaner without those options once ppl move out of gogo). See the generation with buf <https://github.com/bwplotka/benchmarks/blob/main/benchmarks/metrics-streaming/buf.gen.yaml#L5>from proto that has "gogo" fields. Note one (trivial) hack <https://github.com/bwplotka/benchmarks/blob/38a20655422b0152a4953edc288c1edfcca25500/benchmarks/metrics-streaming/Makefile#L14>, discussed here <https://github.com/bwplotka/benchmarks/blob/38a20655422b0152a4953edc288c1edfcca25500/benchmarks/metrics-streaming/Makefile#L14> . > P.S.: This would depend on a change in prometheus/client_model, but would allow removing the duplicate proto definition in the repository. It's unfortunate we have the same client model proto file (and different generated code) in (1) Prometheus <https://github.com/prometheus/prometheus/blob/main/prompb/io/prometheus/client/metrics.proto> (with gogo fields) and in (2) client_model <https://github.com/prometheus/client_model/blob/master/io/prometheus/client/metrics.proto>. Recently we decided to introduce a completely custom unmarshal for textparse proto parser as even gogo was too slow (due to parser interface mostly). <https://github.com/prometheus/prometheus/pull/15731> I planned to write a blog post and perhaps even a Go generator for this, but this means we can move the (1) generated code to an internal package (make it internal to textparse only). For proto file (1) is what is shipped to buf registry as a source of truth <https://buf.build/prometheus/client-model> so I would propose: 1. We make (1) source of truth for proto file. 2. We deprecate (2) client_model and ask for all users to generate themselves the Go code using buf and plugin of their choice for future uses (even gogo if they want). This also means moving out of (2) client_model in client_golang which I think is possible even in 1.x client_golang (e.g. Write2(*ourown.Metric), see a quick exploration here <https://github.com/prometheus/client_golang/pull/1746#issue-2865853407>.). I will create an official proposal for this. > I tried benchmarking BenchmarkSampleSend and the results seem awful, from > ~150000 B/op and ~250 allocs/op to ~1400000 B/op to ~170000 allocs/op. Profiling the code, it seems like labelsToLabelsProto is the offender, as it allocates a *prompb.Label per label. I experimented with using a map[string]string instead, but it seems like the results aren't that much different. We don't have this problem with Remote Write 2.0 BTW, but indeed 1.0 would suffer for now. > One potential solution to this - which would require a relatively big refactor - would be to replace repeated Label labels with the raw byte representation that is held inside of labels_stringlabels, and switch from an array of Sample to two arrays of Timestamp and Value. Just doing the former seems to speed up the benchmarks by 30% (while allocations are still huge because of the *prompb.Sample). It's tempting to make a tailored protocol (proto file) with optimizations that work well for Go. But it's not a silver bullet as we might be making it worse for other languages. Ideally we work in the community on Go decoders and encoders that work well with the current protocol and as we saw with gogo protobuf--it's possible to deliver the optimizations, just it's hard to sustain a generic generator that works for all small cases. During the custom decoder work for client_model proto parsing <https://github.com/prometheus/prometheus/pull/15731> (which is streaming(!), have reusable structs, non-nullable by default (gogo style with less objects) and custom parsing to native types) I learned that: 1) It's not that difficult to write a decoding and encoding of the protobuf <https://github.com/prometheus/prometheus/blob/main/prompb/io/prometheus/client/decoder.go#L173> (by hand or generation). We might exaggerate the amount of work to maintain simple decoding/encoding generation for simple use cases, given gogo trauma. 2) I'm skeptical the main golang protobuf generation will suit high efficiency needs for the data-intensive cases like we have in streaming observability data. With extra plugins or not (vtproto, csproto), the direction of the main golang protobuf generator is to be stable, generically usable and efficient enough for masses. This is why with the new OpaqueAPI <https://go.dev/blog/protobuf-opaque> we have even more pointers (e.g. []*Elem is not *[]*Array, lazy decoding adds more fields and data to the structs), we can't customise unmarshal/marshal easily and generally generates structs are bigger themselves (unknown fields, lazy decoding, metadata handling) <https://github.com/bwplotka/benchmarks/blob/db942ed12cc1b0ab160fbfb882fc3c1b81c0bd98/benchmarks/metrics-streaming/io/prometheus/write/v2/types.pb.go#L509> which is probably fine generally, but for us it's an overhead. What was the last time our decoding code "checked" what unknown fields we received in the proto? Never - so why we have this fat? I think we should write slim Go generator for data intensive proto use and make it easy to write generators here, long term. FYI: OpenTelemetry is discussing similar. <https://github.com/open-telemetry/opentelemetry-collector/issues/7095> Kind Regards, Bartek Plotka (@bwplotka) On Thursday, May 16, 2024 at 3:58:58 PM UTC+1 Bryan Boreham wrote: > > Any reason samples are still encoded as a list of pairs? Using two > arrays would both reduce the number of objects and allow using varint > encoding for timestamps. > > Just to note that Prometheus is coded to send just one sample per series > in a remote-write message. > In normal usage we want to send data straight away after it was scraped, > so that fits. > > Other clients, or Prometheus in some different mode, could benefit as you > say. > > Bryan > > On Wednesday 6 March 2024 at 11:26:50 UTC bjo...@rabenste.in wrote: > >> On 02.03.24 08:00, Mirco wrote: >> > Any reason samples are still encoded as a list of pairs? Using two >> arrays >> > would both reduce the number of objects and allow using varint encoding >> for >> > timestamps. >> >> I assume you are referring to the protobuf messages for TimeSeries and >> Sample? >> >> I'm not an expert for remote write (and even less so for remote read), >> but I think it's safe to say that a change in the protobuf layout >> would mean a new major version of the protocols. v2 is just underway, >> so that would require the next major version bump to v3, which would >> be a big deal given how widely used the protocol is. >> >> Having said that, at least for remote write, there are usually not a >> lot of samples in a TimeSeries message. The most common number is >> AFAIK one. Mileage might vary for remote read, but that's also far >> less used. >> >> WRT varint: In my understanding of protobuf, varint will always be >> used for an int64, even if it is a field in another message. >> >> -- >> Björn Rabenstein >> [PGP-ID] 0x851C3DA17D748D03 >> [email] bjo...@rabenste.in >> > -- You received this message because you are subscribed to the Google Groups "Prometheus Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to prometheus-developers+unsubscr...@googlegroups.com. To view this discussion visit https://groups.google.com/d/msgid/prometheus-developers/66529c70-6514-4ba8-9d1d-b842f7711ebdn%40googlegroups.com.
