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.

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

Influx push handler #1971

Closed
wants to merge 3 commits into from
Closed

Influx push handler #1971

wants to merge 3 commits into from

Conversation

colega
Copy link
Contributor

@colega colega commented May 31, 2022

What this PR does

Adds an http handler that accepts writes in the Influx Line protocol. This code is brought from https://github.com/grafana/influx2cortex with some minor adjustments.

Which issue(s) this PR fixes or relates to

None yet, internal discussion.

Checklist

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

colega added 2 commits May 31, 2022 15:48
Brought code from https://github.com/grafana/influx2cortex and adapted
it to Mimir's httpgrpc errors.

Removed the metrics that the original code had (why would we want more
metrics for influx than for mimir itself?).

Signed-off-by: Oleg Zaytsev <[email protected]>
It will be on /api/v1/influx/push.

Not registering it on the ingesters.

Signed-off-by: Oleg Zaytsev <[email protected]>
@colega
Copy link
Contributor Author

colega commented May 31, 2022

The code here is easy (pending to add some e2e tests which would be straightforwards), it's more about the conversation about whether we want this to be achieved this way (i.e., having this code directly in Mimir).

IMO, this is the way we should go, given the simplicity of the implementation and maintenance.

@pstibrany
Copy link
Member

I agree with you that integrating with Mimir would make it easy to use. But it also adds maintenance burden and expands Mimir in a way that's not compatible with its primary focus ("Grafana Mimir is an open source, horizontally scalable, highly available, multi-tenant, long-term storage for Prometheus.").

We had similar discussion here: cortexproject/cortex#3708

@pracucci
Copy link
Collaborator

pracucci commented Jun 1, 2022

expands Mimir in a way that's not compatible with its primary focus ("Grafana Mimir is an open source, horizontally scalable, highly available, multi-tenant, long-term storage for Prometheus.").

Disagree. Mimir is not Cortex. Mimir wants to be a "open source, horizontally scalable, highly available, multi-tenant, time series database". We're abandoning the "for Prometheus" in our story and we want to be able to ingest your metrics, whatever protocol you want to use.

@pstibrany
Copy link
Member

pstibrany commented Jun 1, 2022

expands Mimir in a way that's not compatible with its primary focus ("Grafana Mimir is an open source, horizontally scalable, highly available, multi-tenant, long-term storage for Prometheus.").

Disagree. Mimir is not Cortex. Mimir wants to be a "open source, horizontally scalable, highly available, multi-tenant, time series database". We're abandoning the "for Prometheus" in our story and we want to be able to ingest your metrics, whatever protocol you want to use.

Screenshot 2022-06-01 at 10 53 25

We should drop "for Prometheus" from our webpage in that case.

Right now there is an effort to open-source our proxies. Having some protocols support in proxies and others directly in Mimir is confusing.

Personally I prefer proxies approach as it's more flexible and allows different backends too (if coded using public API only). But as as Oleg points out, it's also not as convenient and simple to use. (That said, if we decide to have everything in Mimir, I'm happy to commit to that approach.)

Signed-off-by: Oleg Zaytsev <[email protected]>
@56quarters
Copy link
Contributor

I agree with Peter that using one approach for some protocols and another for other protocols is confusing.

FWIW, in GEM we use the following approach:

  • The code for implementing each protocol lives in a separate repo, maintained by the query team.
  • That code is added to GEM as a dependency.
  • Each supported proxy is started within GEM as a module with its own corresponding target, e.g. -target=graphite-write.

I think this is a decent balance between ease of use for users and ease of maintenance for us.

@colega
Copy link
Contributor Author

colega commented Jun 15, 2022

I think this is a decent balance between ease of use for users and ease of maintenance for us.

Vendoring code doesn't simplify maintenance but only makes it more complicated, while end users don't really care if code comes from vendor/ or from pkg/

@56quarters
Copy link
Contributor

I think this is a decent balance between ease of use for users and ease of maintenance for us.

Vendoring code doesn't simplify maintenance but only makes it more complicated, while end users don't really care if code comes from vendor/ or from pkg/

My point was more that the query squad would maintain it and be able to iterate on it as they please, the code in Mimir would only be responsible for wiring up the configuration for it.

@colega colega closed this Sep 28, 2022
alexgreenbank added a commit that referenced this pull request Dec 6, 2024
Signed-off-by: alexgreenbank <[email protected]>
colega added a commit that referenced this pull request Jan 17, 2025
* olegs base commits from #1971

Signed-off-by: alexgreenbank <[email protected]>

* move top level influx files

Signed-off-by: alexgreenbank <[email protected]>

* latest wip

Signed-off-by: alexgreenbank <[email protected]>

* still WIP but better, still need to move to parserFunc() style

Signed-off-by: alexgreenbank <[email protected]>

* it builds!

Signed-off-by: alexgreenbank <[email protected]>

* tweaks and add span logging

Signed-off-by: alexgreenbank <[email protected]>

* more todo

Signed-off-by: alexgreenbank <[email protected]>

* further tweaks

Signed-off-by: alexgreenbank <[email protected]>

* some fixes to tests

Signed-off-by: alexgreenbank <[email protected]>

* rejigged error handling, tests passing

Signed-off-by: alexgreenbank <[email protected]>

* add vendored influxdb code

Signed-off-by: alexgreenbank <[email protected]>

* lint

Signed-off-by: alexgreenbank <[email protected]>

* go mod sum vendor/modules.txt

Signed-off-by: alexgreenbank <[email protected]>

* add a metric, add tenant info, other tweaks

Signed-off-by: alexgreenbank <[email protected]>

* various rework, still WIP

Signed-off-by: alexgreenbank <[email protected]>

* propagate bytesRead down to caller and log and histogram

Signed-off-by: alexgreenbank <[email protected]>

* remove comment now dealt with

Signed-off-by: alexgreenbank <[email protected]>

* add defaults in error handling

Signed-off-by: alexgreenbank <[email protected]>

* Add note to docs about experimental Influx flag

Signed-off-by: alexgreenbank <[email protected]>

* Note influx endpoint as experimental too

Signed-off-by: alexgreenbank <[email protected]>

* test for specific errors received

Signed-off-by: alexgreenbank <[email protected]>

* bolster parser tests

Signed-off-by: alexgreenbank <[email protected]>

* Use literal chars rather than ascii codes

Signed-off-by: alexgreenbank <[email protected]>

* remove unnecessary cast to int()

Signed-off-by: alexgreenbank <[email protected]>

* use mimirpb.PreallocTimeseries in influx parser

Signed-off-by: alexgreenbank <[email protected]>

* remove unnecessary tryUnwrap()

Signed-off-by: alexgreenbank <[email protected]>

* Work on byteslice rather than chars

Signed-off-by: alexgreenbank <[email protected]>

* yoloString for label value as push code does not keep references to strings from LabelAdapter

Signed-off-by: alexgreenbank <[email protected]>

* update go.sum

Signed-off-by: alexgreenbank <[email protected]>

* gah go.sum

Signed-off-by: alexgreenbank <[email protected]>

* oops, missed removal of paramter to InfluxHandler()

Signed-off-by: alexgreenbank <[email protected]>

* wrong metrics incremented

Signed-off-by: alexgreenbank <[email protected]>

* lint

Signed-off-by: alexgreenbank <[email protected]>

* lint

Signed-off-by: alexgreenbank <[email protected]>

* go mod tidy && go mod vendor

Signed-off-by: alexgreenbank <[email protected]>

* go.sum conflict

Signed-off-by: alexgreenbank <[email protected]>

* make doc

Signed-off-by: alexgreenbank <[email protected]>

* make influx config hidden/experimental

Signed-off-by: alexgreenbank <[email protected]>

* fix byteslice handling in replaceInvalidChars()

Signed-off-by: alexgreenbank <[email protected]>

* remove unnecessary TODOs

Signed-off-by: alexgreenbank <[email protected]>

* influx: happy path e2e test

Signed-off-by: alexgreenbank <[email protected]>

* lint

Signed-off-by: alexgreenbank <[email protected]>

* consolidate logging

Signed-off-by: alexgreenbank <[email protected]>

* CHANGELOG

Signed-off-by: alexgreenbank <[email protected]>

* about-versioning.md

Signed-off-by: alexgreenbank <[email protected]>

* Update pkg/distributor/influxpush/parser.go

Co-authored-by: Oleg Zaytsev <[email protected]>

* Update pkg/distributor/influxpush/parser.go

Co-authored-by: Oleg Zaytsev <[email protected]>

* Update pkg/distributor/influxpush/parser.go

Co-authored-by: Oleg Zaytsev <[email protected]>

* fix parsing string replacing code

Signed-off-by: alexgreenbank <[email protected]>

* fix nits

Signed-off-by: alexgreenbank <[email protected]>

---------

Signed-off-by: alexgreenbank <[email protected]>
Co-authored-by: Oleg Zaytsev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants