-
Notifications
You must be signed in to change notification settings - Fork 455
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
[coordinator] Add support for prometheus remote write compatible backend #3742
Conversation
d783d29
to
7c19144
Compare
Codecov Report
@@ Coverage Diff @@
## master #3742 +/- ##
========================================
- Coverage 57.0% 57.0% -0.1%
========================================
Files 552 552
Lines 62950 62950
========================================
- Hits 35919 35913 -6
- Misses 23840 23841 +1
- Partials 3191 3196 +5
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few initial comments, will continue reviewing tomorrow.
6b64ad3
to
b37164e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, though it would be best if someone else would take a look, too.
metrics.ReportError(methodDuration) | ||
response, err := ioutil.ReadAll(resp.Body) | ||
if err != nil { | ||
response = []byte(fmt.Sprintf("error reading body: %v", err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you can return a generic error now, and TODO just logging.
…f address since its unique for metrics
b8a3265
to
8157671
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except (outside of nits/minor comments) I do feel relatively strongly that we return an error when retention and resolution doesn't match any of the endpoints listed before we merge this (referencing this comment here https://github.com/m3db/m3/pull/3742/files#r713168023)
What this PR does / why we need it:
Adds support for new backend type - prometheus remote write. This enables using M3 Coordinator and M3 Aggregator with Prometheus Remote Write compatible API.
Special notes for your reviewer:
Read part is not implemented mainly because using remote read API is not efficient since it requires to load raw timeseries.
In this PR integration with aggregator is not fully implemented. For now one can specify endpoints with no storage policy only so they will receive raw writes.
I plan to fix this in subsequent PR.
Example of configuration when data aggregation will be available:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: