Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add remote write protocol 2.0 support #9072

Open
Tracked by #7817
dimitarvdimitrov opened this issue Aug 22, 2024 · 4 comments
Open
Tracked by #7817

Add remote write protocol 2.0 support #9072

dimitarvdimitrov opened this issue Aug 22, 2024 · 4 comments
Assignees
@aknuds1
Labels
component/distributor component/ingester enhancement New feature or request

Comments

@dimitarvdimitrov
Copy link
Contributor

dimitarvdimitrov commented Aug 22, 2024 •
edited by armandgrillet
Loading

What should we do?

Prometheus added remote write protocol 2.0 experimental support in v2.54.0 (released on 2024-08-09). We should add the support in Mimir too.

How will we do it (roughly)?

  • Add remote write 2.0 support in Mimir distributors
  • Backport all (applicable) optimizations we did to remote write 1.0 (un)marshalling
  • Compare performance between remote write 1.0 and 2.0
  • Allow to enable 2.0 support on a per-tenant basis

Private design doc: https://docs.google.com/document/d/1JSwhdWRODOeGlNIRpYvnEHK6aH7d42n4ZJ_rfFvt-Lo/edit?tab=t.0#heading=h.5sybau7waq2q

Out of the scope of this work:

  • Change data format between distributors and ingesters / Kafka (keep using protocol 1.0 format). This should be a follow up deliverable.

Size?

Between Medium (= ~1 month) and Large (= ~3 month).

What will we deliver?

  • Add remote write 2.0 experimental support in Mimir, fully merged in Mimir but disabled by default
  • Test remote write 2.0 in dev

What are the documentation dependencies?

  • No documentation changes to docs until enabled by default

Urgency?

Not urgent yet, but we can't lag too much behind Prometheus
@pstibrany pstibrany mentioned this issue Aug 22, 2024
Closed
@jmichalek132
Copy link
Contributor

FYI otel collector contrib has lfx mentorship project starting in September to add support in the remote write exporter for remote write 2.0. Tracking issue: open-telemetry/opentelemetry-collector-contrib#33661.

@krajorama krajorama mentioned this issue Jan 6, 2025
Open
@krajorama
Copy link
Contributor

This is becoming a bit more urgent now due to two projects depending on it: native histograms with custom buckets (NHCB) and proper handling of OTLP created timestamp (current solution we don't want to upstream because it's a bit of a workaround).

This was referenced Jan 9, 2025
Merged
Merged
krajorama added a commit to prometheus/prometheus that referenced this issue Jan 14, 2025
@krajorama
fix(remotewrite2): do not overwrite help text with unit in enqueue
594b882 
While testing POC for grafana/mimir#9072
I saw no unit or help metadata. Our test env:
https://github.com/grafana/mimir/tree/main/development/mimir-monolithic-mode
doesn't have units, so that was empty and cleared the help due to this bug.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
@krajorama krajorama mentioned this issue Jan 14, 2025
Merged
krajorama added a commit to prometheus/prometheus that referenced this issue Jan 15, 2025
@krajorama
fix(remotewrite2): do not send uninitialized garbage if there's no me…
482a7fd 
…tadata

Found during testing for
grafana/mimir#9072

Debug printout showed:
KRAJO: seriesName=cortex_request_duration_seconds_bucket,
metricFamily=cortex_request_duration_seconds_bucket,
type=GAUGE,
help=cortex_bucket_index_load_duration_seconds_sum,
unit=

which is nonsense.

I can imagine more cases where this is the case and makes actual sense.
Some targets might miss metadata and if there's a pipeline that loses it.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
This was referenced Jan 15, 2025
Merged
Draft
krajorama added a commit that referenced this issue Jan 16, 2025
@krajorama
refactor(mimirpb): delete TestRemoteWriteV1HistogramEquivalence
267c7e0 
As far as I can tell we don't cast Prometheus Remote Write 1.0
histogram into mimirpb.Histogram anymore. On the flip-side
this test fails in #10432 because we're going to store RW 2.0
extra field in mimirpb.Histogram.

Related to #9072

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
@krajorama krajorama mentioned this issue Jan 16, 2025
Merged
krajorama added a commit that referenced this issue Jan 16, 2025
@krajorama
refactor(mimirpb): delete TestRemoteWriteV1HistogramEquivalence
a864aa3 
As far as I can tell we don't cast Prometheus Remote Write 1.0
histogram into mimirpb.Histogram anymore. On the flip-side
this test fails in #10432 because we're going to store RW 2.0
extra field in mimirpb.Histogram.

Related to #9072

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
@krajorama
Copy link
Contributor

Based on #10432

Task list:

  • Remove/refactor debug statements in PreallocWriteRequest.unmarshalRW2
  • Add unit test for PreallocWriteRequest.unmarshalRW2
  • Add benchmark test for PreallocWriteRequest.unmarshalRW2
  • Add configuration option to enable/disable RW2.0 support. If disabled it should return 415 as in fix(rw2.0): reject remote write 2.0 based on content type #10423
  • Add metric to measure which protocol is in use
  • Add more testcases to TestDistributorRemoteWrite2 integration test
  • Patch mimir.pb.go to ignore RW2 only fields when unmarshal RW1
  • Add Changelog entry
  • Run Prometheus tool that checks compatibility against out endpoint if possible
  • Performance test with avalanche

Vandit1604 pushed a commit to Vandit1604/prometheus that referenced this issue Jan 16, 2025
@krajorama @Vandit1604
fix(remotewrite2): do not overwrite help text with unit in enqueue
ba76432 
While testing POC for grafana/mimir#9072
I saw no unit or help metadata. Our test env:
https://github.com/grafana/mimir/tree/main/development/mimir-monolithic-mode
doesn't have units, so that was empty and cleared the help due to this bug.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Vandit1604 pushed a commit to Vandit1604/prometheus that referenced this issue Jan 16, 2025
@krajorama @Vandit1604
fix(remotewrite2): do not send uninitialized garbage if there's no me…
e02237f 
…tadata

Found during testing for
grafana/mimir#9072

Debug printout showed:
KRAJO: seriesName=cortex_request_duration_seconds_bucket,
metricFamily=cortex_request_duration_seconds_bucket,
type=GAUGE,
help=cortex_bucket_index_load_duration_seconds_sum,
unit=

which is nonsense.

I can imagine more cases where this is the case and makes actual sense.
Some targets might miss metadata and if there's a pipeline that loses it.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
krajorama added a commit that referenced this issue Jan 17, 2025
@krajorama
refactor(mimirpb): delete TestRemoteWriteV1HistogramEquivalence (#10450)
504dd37 
As far as I can tell we don't cast Prometheus Remote Write 1.0
histogram into mimirpb.Histogram anymore. On the flip-side
this test fails in #10432 because we're going to store RW 2.0
extra field in mimirpb.Histogram.

Related to #9072

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
@francoposa
Copy link
Member

francoposa commented Feb 5, 2025 •
edited
Loading

Just want to raise that I have done a bunch of investigation on performance and viability of replacing Gogo and remote write v2 is easily our best chance to have the next generation of protos here move off of Gogo.

The stuff we have hand-rolled manually hacked into the Gogo codegen, with yoloString, memory pooling and pre-allocation is fully supported now in libraries like vtprotobuf which are compatible with upstream (unlike Gogo), and vtprotobuf is adding support to implement grpc-go's CodecV2 with mempool support in the grpc servers.

So the potential exists to get the same or better performance with a fraction of the maintenance that comes from using the custom types and manually changing copy-pasted marshaling methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aknuds1 aknuds1

Labels
component/distributor component/ingester enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@aknuds1 @jmichalek132 @francoposa @krajorama @dimitarvdimitrov