Prometheus Remote Write protocol support#8702

Closed

aleksey-novikov wants to merge 3 commits intoinfluxdata:masterfrom

aleksey-novikov:master

aleksey-novikov commented Jan 15, 2021 •

edited by sjwang90

Loading

Prometheus Remote Write protocol

This is basic implementation of Prometheus Remote Write Protocol. Includes:

Snappy Content-Encoding support
Prometheus Remote Write parser

Must be used with http_listener_v2

Sample input configuration:

[[inputs.http_listener_v2]]
  service_address = ":10000"
  data_format = "prometheusremotewrite"

Closes #8682

av.novikov1 added 2 commits

January 15, 2021 09:44


          Add snappy encoded bodies support

b7fac16


          Implement Prometheus Remote Write Parser

a3f26a6

telegraf-tiger bot requested changes

View reviewed changes

Contributor

telegraf-tiger bot left a comment

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

aleksey-novikov mentioned this pull request

Add Prometheus Remote Write Parser #8682

Closed

lgtm-com bot commented Jan 15, 2021

This pull request introduces 1 alert when merging a3f26a6 into 76c2201 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable


          cleanup

da4f1cc

Contributor

sjwang90 commented Jan 15, 2021

Fixes #8682

sjwang90 linked an issue

that may be closed by this pull request

Add Prometheus Remote Write Parser #8682

Closed

sjwang90 added area/prometheus plugin/parser labels

Hipska added the new plugin label

srebhan reviewed

View reviewed changes

plugins/parsers/prometheusremotewrite/parser.go

Comment on lines +11 to +13

+              	"github.com/prometheus/prometheus/prompb"
+              	"github.com/gogo/protobuf/proto"
+              	"github.com/prometheus/common/model"

Member

srebhan Jan 26, 2021

Please order alphabetically.

srebhan reviewed

View reviewed changes

plugins/parsers/prometheusremotewrite/parser.go

Comment on lines +24 to +25

Member

srebhan Jan 26, 2021

Please just one newline. Same for all of those instances below.

srebhan reviewed

View reviewed changes

plugins/parsers/prometheusremotewrite/parser.go

Comment on lines +52 to +53

		metric, err := metric.New("prometheusremotewrite", tags, fields, t)
		if err == nil {

Member

srebhan Jan 26, 2021

We might silently drop metrics here without allowing the user to be aware of this. Please either fail here (i.e. return err) or at least issue a log message in case we see an error here.

srebhan reviewed

View reviewed changes

plugins/parsers/prometheusremotewrite/parser.go

+              			tags[l.Name] = l.Value
+              		}
+              		metricName := tags[model.MetricNameLabel]

Member

srebhan Jan 26, 2021 •

edited

Loading

Can we be absolutely sure that tag exists? I will sleep better if we check that model.MetricNameLabel exists in tags by

Suggested change

      
            		metricName := tags[model.MetricNameLabel]
          
            		metricName := tags[model.MetricNameLabel]
          
            		if metricName == "" {
          
            			return fmt.Errorf("metric name %q not found in tag-set or empty", model.MetricNameLabel)
          
            		}

srebhan reviewed

View reviewed changes

plugins/parsers/prometheusremotewrite/parser.go

+              		metricName := tags[model.MetricNameLabel]
+              		delete(tags, model.MetricNameLabel)
+              		for _, s := range ts.Samples {

Member

srebhan Jan 26, 2021

As we approximately know how many metrics we expect we might want to preallocate the array with the number of samples, i.e. capacity = len(ts.Samples) and len = 0.

srebhan reviewed

View reviewed changes

plugins/parsers/prometheusremotewrite/parser.go

+              		for _, l := range ts.Labels {
+              			tags[l.Name] = l.Value
+              		}

Member

srebhan Jan 26, 2021

You also have to add the DefaultTags here.

srebhan reviewed

View reviewed changes

plugins/parsers/prometheusremotewrite/parser.go

+              		delete(tags, model.MetricNameLabel)
+              		for _, s := range ts.Samples {
+              			fields := getNameAndValue(&s, metricName)

Member

srebhan Jan 26, 2021

Maybe you should move the code here. It's not a lot and this would follow the tags handling.

srebhan reviewed

View reviewed changes

plugins/parsers/prometheusremotewrite/parser.go

+              			// converting to telegraf metric
+              			if len(fields) > 0 {
+              				t := getTimestamp(&s, now)

Member

srebhan Jan 26, 2021

You might also move the body of the function here. See above.

srebhan requested changes

View reviewed changes

Member

srebhan left a comment

Hey @aleksey-novikov, this is really nice code! I have some comments in the code. Additionally I have some major comments/requests:

Please split the http_listener, snappy encoding change into an own PR. I think this is independent of the prometheus parser a very useful change.
Please add tests! This holds for both, the snappy support as well as the parser part. Provide some examples where you know the expected output.
This is more a question/suggestion rather than a request: Do you think it makes sense to collect samples from the same timestamp into one metric? Not sure if this makes sense at all, but I want to know your opinion on this...
You should add a README.md with some parsing result examples. Furthermore, you need to extend the list of available parsers etc.

srebhan self-assigned this

Contributor

sjwang90 commented Feb 16, 2021

Hi @aleksey-novikov 👋 ,

Just checking if you can address @srebhan's comments. We'd love to get this in our upcoming 1.18 feature release targeted for the end of March.

Contributor

sjwang90 commented Feb 26, 2021

@aleksey-novikov - Another ping. We'd love to get this into 1.18 so let us know if you want to make the changes otherwise someone on the influx side can finish it up. Thanks!

Member

helenosheaa commented Mar 3, 2021 •

edited

Loading

Hey @aleksey-novikov great work! We're really interested on working to get this merged in. Are you interested in carrying on with the PR? If you could address some of @srebhan's comments. If not I can take over the PR and get it to a ready to merge state to be merged.

helenosheaa self-assigned this

Author

aleksey-novikov commented Mar 4, 2021

Hi @helenosheaa!
Unfortunately, I do not have enough experience in golang development and do not have enough time to adapt these changes. I needed to implement the collection of metrics from Kubernetes into the our company's monitoring system based on Kafka + Graphite + ClickHouse. As tests have shown, the Telegraf creates a significantly higher load on the CPU than Prometheus. Therefore, I implemented the functionality of transferring metrics from Prometheus to a dedicated stand-alone Telegraf instance. In our case, it works, it seems even quite stable. If someone is involved in the maintenance of this PR, I will be glad of it.

Member

helenosheaa commented Mar 4, 2021

Hi @aleksey-novikov, thanks for getting back to me quickly. I'll start my own PR and link to this one and close when it's up. Thanks for getting it going and if you have any feedback when I submit mine please comment.

helenosheaa mentioned this pull request

New prometheus remote write parser #8967

Merged

Member

helenosheaa commented Mar 10, 2021

Closing to move forward with #8967

helenosheaa closed this

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

Labels

area/prometheus new plugin plugin/parser