Add PushOnlyIngester gRPC service; Disable label name validation #3736

trevorwhitney · 2021-01-22T22:35:35Z

What this PR does:
This PR enables users who want to store non-conventional prometheus data in cortex by exposing the ability to disable label name validation via a request parameter. This option is currently available to in the distributor config, but that requires you to run a whole separate distributor to handle requests for this type. By exposing this option through the WriteRequest, a single distributor can handle both payload types.

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

jtlisi

LGTM, modulo linting fix

jtlisi · 2021-01-23T02:15:09Z

pkg/distributor/distributor_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"google.golang.org/grpc/status"


This import needs to be separate from the standard library imports. We run rather strict linter settings for the project and this is triggering the CI failure.

ahh, good to know, thanks!

just a bit more detail about the linting: https://cortexmetrics.io/docs/contributing/#formatting

roidelapluie · 2021-01-23T13:45:13Z

What is the usecase here? I expect promql not to work without proper metric names.

roidelapluie · 2021-01-23T13:47:12Z

I also quote @pstibrany

Now the question is, do we want to be a generic TSDB? I don't know, I want it to be.

I don't think Cortex should try to be a generic TSDB. There are already projects that do that better, and Cortex' strength is in making Prometheus horizontally-scalable (wherever that eventually leads). If there is low-hanging fruit to make some use-cases easier, I'm all for it, but generic TSDB is completely different beast altogether (starting with query language, underlying storage, architecture choices).

#3708

pstibrany · 2021-01-25T07:30:27Z

What is the usecase here? I expect promql not to work without proper metric names.

As far as I understand it, the use case here is to add a support for storing a non-Prometheus monitoring data into Cortex. This would be achieved by 1) having a proxy for incoming data that would sit in front of distributor, and call Push on distributor via gRPC with this new flag set to true (skip validation). 2) On query time, there would be a special querier that doesn't use PromQL at all, but use Cortex' store-gateway to fetch required data.

Additional support in Cortex needed to make this work is pretty small: disable label validation (this PR), and expose distributor push via gRPC. I think this falls well within "low-hanging fruit to make some use-cases easier".

pstibrany · 2021-01-25T08:20:10Z

pkg/ingester/client/cortex.proto

@@ -33,6 +33,7 @@ message WriteRequest {
  }
  SourceEnum Source = 2;
  repeated MetricMetadata metadata = 3 [(gogoproto.nullable) = true];
+  bool skip_label_name_validation = 4;


I'm wondering if we should use google.protobuf.Any hints approach here. Strictly speaking this isn't supported by Ingester service, so it doesn't belong here. But Distributor implements the same method. (When we expose Distributor via gRPC, I expect it will reuse the same Push method, with cortex.WriteRequest and cortex.WriteResponse)

We need to keep WriteRequest compatible with Prometheus one. If we use the field number 4, it's very likely it will clash next time Prometheus remote write adds a new field. To fix this, we could use a very high field number (eg. 1000).

@pstibrany is the google.protobuf.Any hints approach like the options field in the rules proto (

cortex/pkg/ruler/rules/rules.proto

Line 29 in d625141

repeated google.protobuf.Any options = 9;

), making this field extensible in the case of future config options? Or am I missing the boat here?

pstibrany · 2021-01-25T08:22:55Z

pkg/distributor/distributor.go

@@ -366,9 +366,9 @@ func (d *Distributor) checkSample(ctx context.Context, userID, cluster, replica
 // Validates a single series from a write request. Will remove labels if
 // any are configured to be dropped for the user ID.
 // Returns the validated series with it's labels/samples, and any error.
-func (d *Distributor) validateSeries(ts ingester_client.PreallocTimeseries, userID string) (client.PreallocTimeseries, error) {
+func (d *Distributor) validateSeries(ts ingester_client.PreallocTimeseries, userID string, skipLabelNameValidation bool) (ingester_client.PreallocTimeseries, error) {


Nice catch! I've sent separate PR #3738 to fix double-import of ingester_client package in this file.

We've just merged #3738. Could you rebase master, please?

pracucci · 2021-01-25T10:18:20Z

pkg/ingester/client/cortex.proto

@@ -33,6 +33,7 @@ message WriteRequest {
  }
  SourceEnum Source = 2;
  repeated MetricMetadata metadata = 3 [(gogoproto.nullable) = true];
+  bool skip_label_name_validation = 4;


We need to keep WriteRequest compatible with Prometheus one. If we use the field number 4, it's very likely it will clash next time Prometheus remote write adds a new field. To fix this, we could use a very high field number (eg. 1000).

pracucci · 2021-01-25T10:19:07Z

pkg/distributor/distributor.go

@@ -366,9 +366,9 @@ func (d *Distributor) checkSample(ctx context.Context, userID, cluster, replica
 // Validates a single series from a write request. Will remove labels if
 // any are configured to be dropped for the user ID.
 // Returns the validated series with it's labels/samples, and any error.
-func (d *Distributor) validateSeries(ts ingester_client.PreallocTimeseries, userID string) (client.PreallocTimeseries, error) {
+func (d *Distributor) validateSeries(ts ingester_client.PreallocTimeseries, userID string, skipLabelNameValidation bool) (ingester_client.PreallocTimeseries, error) {


We've just merged #3738. Could you rebase master, please?

pracucci · 2021-01-25T10:21:32Z

pkg/distributor/distributor.go

@@ -495,7 +495,8 @@ func (d *Distributor) Push(ctx context.Context, req *client.WriteRequest) (*clie
 			return nil, err
 		}

-		validatedSeries, err := d.validateSeries(ts, userID)
+		skipLabelNameValidation := d.cfg.SkipLabelNameValidation || req.GetSkipLabelNameValidation()


@replay SkipLabelNameValidation was added to the config in #2835, based on needs of a downstream project. Do we still need it in the config or can we remote it favour of specifying it directly in the request? If it's needed in the config too, then OK, otherwise I would prefer to keep code cleaner.

You're right, we won't need the SkipLabelNameValidation in the config struct anymore.

Would it be fine for you if we remove that flag from the Distributor config in a separate PR after this one has been merged? That would help because otherwise merging this PR is going to break the down-stream CI tests of projects relying on the Distributor config flag, if we remove it in a separate PR we can first switch the down-stream users to rely on this new per-request flag and only after that we remove the Distributor config flag.

I'm fine if you will do it in a separate PR.

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

* distributor needs to be registered with grpc server so that other services have this new validation bypass option when talking to distributor directly * util used to translate http -> grpc always sets SkipLabelNameValidation to false to force validation on all http requests as only gRPC requests should be allowed to bypass this validation Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

trevorwhitney · 2021-01-25T19:56:50Z

Thanks everyone, just pushed up a commit that hopefully resolves above mentioned issues.

created a new grpc service with just the Push method to expose the distributor via grpc
changed the grpc field position in WriteRequest to 1000 to guard against future changes to upstream WriteRequest in prometheus
updated the util/push/push.go to always set the SkipLabelNameValidation property to false so that all http request will continue to be validated.

replay

LGTM

(trevor, note that I'm not a maintainer, so my approval doesn't count towards the requirement of 2 to merge)

trevorwhitney · 2021-01-25T22:44:18Z

haha, @replay appreciate the support nonetheless!

pstibrany

LGTM.

(Nit: it seems little strange that "cortex.Distributor" gRPC service is defined in ingester package)

trevorwhitney · 2021-01-26T17:45:17Z

@pstibrany how would you feel about renaming the new service to PushIngestor instead of Distrubutor, since that's really what it is? That way it would probably make more sense to keep in the ingester package.

pstibrany · 2021-01-26T17:50:57Z

@pstibrany how would you feel about renaming the new service to PushIngestor instead of Distrubutor, since that's really what it is? That way it would probably make more sense to keep in the ingester package.

I don’t have a strong preference, and also I’m fine with keeping it as is.

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

trevorwhitney · 2021-01-26T21:15:06Z

I went ahead with the rename of Distributor service to PushOnlyIngester as I think (as @pstibrany pointed out) that makes more sense for something in the ingester package. I think this is ready for a merge, but I don't have write permission to do so. Thanks!

pracucci

LGTM! Please remind to open a PR to remove the "skip label name validation" from config, since you mentioned you don't need to anymore.

pull-request-size bot added the size/M label Jan 22, 2021

trevorwhitney force-pushed the disable-label-name-validation branch from 406a543 to 7ba6e79 Compare January 22, 2021 22:41

jtlisi approved these changes Jan 23, 2021

View reviewed changes

pstibrany reviewed Jan 25, 2021

View reviewed changes

pstibrany mentioned this pull request Jan 25, 2021

Remove double-import of github.com/cortexproject/cortex/pkg/ingester/client package. #3738

Merged

pracucci reviewed Jan 25, 2021

View reviewed changes

allow label name validation to be skipped via request param

c5dee0a

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

trevorwhitney force-pushed the disable-label-name-validation branch from 7ba6e79 to fe982d5 Compare January 25, 2021 19:49

trevorwhitney force-pushed the disable-label-name-validation branch from fe982d5 to c19420c Compare January 25, 2021 19:51

replay approved these changes Jan 25, 2021

View reviewed changes

pstibrany approved these changes Jan 26, 2021

View reviewed changes

rename Distributor service to PushOnlyIngester

408feff

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

pracucci approved these changes Jan 27, 2021

View reviewed changes

pracucci merged commit e414714 into cortexproject:master Jan 27, 2021

trevorwhitney deleted the disable-label-name-validation branch January 27, 2021 17:14

trevorwhitney mentioned this pull request Jan 29, 2021

skipLabelNameValidation can now be set per request so no longer needed in config #3765

Merged

3 tasks

bboreham changed the title ~~Disable label name validation~~ Add PushOnlyIngester gRPC service; Disable label name validation Feb 25, 2021

bboreham mentioned this pull request Feb 25, 2021

Remote Write GRPC Endpoint #2808

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PushOnlyIngester gRPC service; Disable label name validation #3736

Add PushOnlyIngester gRPC service; Disable label name validation #3736

trevorwhitney commented Jan 22, 2021

jtlisi left a comment

jtlisi Jan 23, 2021

trevorwhitney Jan 25, 2021

replay Jan 27, 2021

roidelapluie commented Jan 23, 2021

roidelapluie commented Jan 23, 2021

pstibrany commented Jan 25, 2021 •

edited

Loading

pstibrany Jan 25, 2021

pracucci Jan 25, 2021

trevorwhitney Jan 25, 2021

pstibrany Jan 25, 2021

pracucci Jan 25, 2021

pracucci Jan 25, 2021

pracucci Jan 25, 2021

pracucci Jan 25, 2021

replay Jan 25, 2021

pracucci Jan 25, 2021

trevorwhitney commented Jan 25, 2021

replay left a comment •

edited

Loading

trevorwhitney commented Jan 25, 2021

pstibrany left a comment

trevorwhitney commented Jan 26, 2021

pstibrany commented Jan 26, 2021

trevorwhitney commented Jan 26, 2021

pracucci left a comment

Add PushOnlyIngester gRPC service; Disable label name validation #3736

Add PushOnlyIngester gRPC service; Disable label name validation #3736

Conversation

trevorwhitney commented Jan 22, 2021

jtlisi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roidelapluie commented Jan 23, 2021

roidelapluie commented Jan 23, 2021

pstibrany commented Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevorwhitney commented Jan 25, 2021

replay left a comment • edited Loading

Choose a reason for hiding this comment

trevorwhitney commented Jan 25, 2021

pstibrany left a comment

Choose a reason for hiding this comment

trevorwhitney commented Jan 26, 2021

pstibrany commented Jan 26, 2021

trevorwhitney commented Jan 26, 2021

pracucci left a comment

Choose a reason for hiding this comment

pstibrany commented Jan 25, 2021 •

edited

Loading

replay left a comment •

edited

Loading