Remove remote read proto definitions from Mimir

[pracucci]

What this PR does

In Mimir we define many protobuf messages. A part from a special case, they're all used for internal Mimir communication, so it's correct that they're defined in Mimir. The special case is remote read.

The remote read endpoint works with protobuf-encoded messages. It receives in input a protobuf-encoded request and returns protobuf-encoded messages. The remote read endpoint specs are defined by Prometheus, which also defines the protobuf messages.

In Mimir, we define our own remote read protobuf messages which are essentially copy-paste of Prometheus one (except for ReadHints, not defined in Mimir). This may cause drifts between Prometheus and Mimir. Given the remote read endpoint is defined by Prometheus, we want to be Prometheus-compatible.

To reduce risks of drifting, I would like to stop using Mimir-defined protobuf messages for the remote read endpoint. couldn't find any good reason to keep them. Prometheus already offers all the functions to convert data types (e.g. histograms) back and forth the protobuf messages.

In this PR I propose to delete Mimir-defined remote read protobuf messages and use Prometheus ones instead.

Note to reviewers:

• Here I've added protobuf comparison between Mimir and Prometheus
• To make remote read easier to manually test, I've configured Prometheus to remote read from Mimir in the local dev env.
• Added remote read requests to backward compatibility integration test

Which issue(s) this PR fixes or relates to

N/A

Checklist

• Tests updated.
• Documentation added.
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
• about-versioning.md updated with experimental features.

[pracucci]

Note to reviewers: previously, QueryRequest was referenced by ReadRequest so it was right that the field 4 (hints) was reserved. After this PR, QueryRequest is just used internally in Mimir and it's used anymore to decode a remote read request, so we don't have any Prometheus-compatibility requirement anymore.

[krajorama]

Please add two tests to remote_read_test.go that make it explicit that the provided Step, StartMs, EndMs hints are ignored. There's already some mock storage.Querier implementations there, should be simple to do. To ensure we're not getting out of sync with assumptions in the frontend.

[pracucci]

Protobuf comparison between Mimir and Prometheus

I've just stripped comments that were not useful for the comparison purpose.

ReadRequest

Mimir:

message ReadRequest {
  repeated QueryRequest queries = 1;

  enum ResponseType {
    SAMPLES = 0;
    STREAMED_XOR_CHUNKS = 1;
  }
  repeated ResponseType accepted_response_types = 2;
}

Prometheus:

message ReadRequest {
  repeated Query queries = 1;

  enum ResponseType {
    SAMPLES = 0;
    STREAMED_XOR_CHUNKS = 1;
  }

  repeated ResponseType accepted_response_types = 2;
}

Query

Mimir:

message QueryRequest {
  reserved 4;
  reserved "hints";

  int64 start_timestamp_ms = 1;
  int64 end_timestamp_ms = 2;
  repeated LabelMatcher matchers = 3;
  
  uint64 streaming_chunks_batch_size = 100;
}

Prometheus:

message Query {
  int64 start_timestamp_ms = 1;
  int64 end_timestamp_ms = 2;
  repeated prometheus.LabelMatcher matchers = 3;
  prometheus.ReadHints hints = 4;
}

NOTE: streaming_chunks_batch_size in Mimir is unused when unmarshalling the remote read request. Prometheus has hints that we didn't support in Mimir. We're still not supporting them in Mimir after this PR.

ReadResponse

Mimir:

message ReadResponse {
  repeated QueryResponse results = 1;
}

Prometheus:

message ReadResponse {
  repeated QueryResult results = 1;
}

QueryResponse

Mimir:

message QueryResponse {
  repeated cortexpb.TimeSeries timeseries = 1 [(gogoproto.nullable) = false];
}

Prometheus:

message QueryResult {
  repeated prometheus.TimeSeries timeseries = 1;
}

TimeSeries

Mimir:

message TimeSeries {
  repeated LabelPair labels = 1 [(gogoproto.nullable) = false, (gogoproto.customtype) = "LabelAdapter"];
  repeated Sample samples = 2 [(gogoproto.nullable) = false];
  repeated Exemplar exemplars = 3 [(gogoproto.nullable) = false];
  repeated Histogram histograms = 4 [(gogoproto.nullable) = false];
}

Prometheus:

message TimeSeries {
  repeated Label labels         = 1 [(gogoproto.nullable) = false];
  repeated Sample samples       = 2 [(gogoproto.nullable) = false];
  repeated Exemplar exemplars   = 3 [(gogoproto.nullable) = false];
  repeated Histogram histograms = 4 [(gogoproto.nullable) = false];
}

LabelPair

Mimir:

message LabelPair {
  bytes name  = 1;
  bytes value = 2;
}

Prometheus:

message Label {
  string name  = 1;
  string value = 2;
}

Sample

Mimir:

message Sample {
	// Fields order MUST match promql.FPoint so that we can cast types between them.
  int64 timestamp_ms = 2;
  double value       = 1;
}

Prometheus:

message Sample {
  double value    = 1;
  int64 timestamp = 2;
}

NOTE: in the case of a remote read response we never cast types, so the change in Sample field ordering doesn't matter.

Exemplar

Mimir:

message Exemplar {
  repeated LabelPair labels = 1 [(gogoproto.nullable) = false, (gogoproto.customtype) = "LabelAdapter"];
  double value = 2;
  int64 timestamp_ms = 3;
}

Prometheus:

message Exemplar {
  repeated Label labels = 1 [(gogoproto.nullable) = false];
  double value = 2;
  int64 timestamp = 3;
}

Histogram

@krajorama Can we skip the histogram comparison? Are they expected to be 1:1 with Prometheus ones, right?

StreamReadResponse

Mimir:

message StreamReadResponse {
  repeated StreamChunkedSeries chunked_series = 1;

  int64 query_index = 2;
}

Prometheus:

message ChunkedReadResponse {
  repeated prometheus.ChunkedSeries chunked_series = 1;

  int64 query_index = 2;
}

StreamChunkedSeries

Mimir:

message StreamChunkedSeries {
  repeated cortexpb.LabelPair labels = 1 [(gogoproto.nullable) = false, (gogoproto.customtype) = "github.com/grafana/mimir/pkg/mimirpb.LabelAdapter"];
  repeated StreamChunk chunks = 2 [(gogoproto.nullable) = false];
}

Prometheus:

message ChunkedSeries {
  repeated Label labels = 1 [(gogoproto.nullable) = false];
  repeated Chunk chunks = 2 [(gogoproto.nullable) = false];
}

StreamChunk

Mimir:

message StreamChunk {
  int64 min_time_ms = 1;
  int64 max_time_ms = 2;

  enum Encoding {
    UNKNOWN = 0;
    XOR     = 1;
    HISTOGRAM = 2;
    FLOAT_HISTOGRAM = 3;
  }
  Encoding type  = 3;
  bytes data     = 4 [(gogoproto.nullable) = false, (gogoproto.customtype) = "github.com/grafana/mimir/pkg/mimirpb.UnsafeByteSlice"];
}

Prometheus:

message Chunk {
  int64 min_time_ms = 1;
  int64 max_time_ms = 2;

  enum Encoding {
    UNKNOWN         = 0;
    XOR             = 1;
    HISTOGRAM       = 2;
    FLOAT_HISTOGRAM = 3;
  }
  Encoding type  = 3;
  bytes data     = 4;
}

Note: Mimir uses UnsafeByteSlice. It's an optimization for the unmarshalling, but we never unmarshal StreamChunk because we just use it to send the response (so it's just marshalled).

[pracucci]

Please add two tests to remote_read_test.go that make it explicit that the provided Step, StartMs, EndMs hints are ignored.

I've added the assertion to existing tests, in 677c9a3. I suggest to review changes with "hide whitespace changes".

[krajorama]

@krajorama Can we skip the histogram comparison? Are they expected to be 1:1 with Prometheus ones, right?

Yes. Although there's no explicit test for it, which is a little strange, I thought we had one. I'm adding a unit test in a different PR.

[krajorama]

Unit test for checking histogram equivalence: checks the names and types and order of fields, except for Gogo proto meta fields: #8426

[krajorama]

LGTM

more work expected

[pracucci]

Note to reviewers: this change is not a fix. It just makes it easier to look at the diff in case the assertion fails, otherwise it could be misleading seeing series_1 both as metric name, and as additional key-value label pair.

[pracucci]

Note to reviewers: moved to integration/util.go, close to the new utilities.

[krajorama]

LGTM, except test is reinventing mocking to some extent, I'll propose a PR