Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add influx push endpoint to mimir #10153
Add influx push endpoint to mimir #10153
Changes from 13 commits
444d34f
adb376a
3db179f
295fa8d
d2897e3
c2fb679
593b2e2
31bf23d
2487a88
03e8e3c
0bd8da8
4a76a11
066c009
bec3a26
ac51def
3a57dc6
92379e4
d44c71d
591389e
afbc357
847bcb9
320c467
730a7c3
de27d4b
af3def1
d65b3a5
9d94276
258fe0d
e5252d4
f86691b
32cc156
c798360
8d4e7ca
e915764
013b3d6
3c5a166
773722f
6143162
ac4e491
767695a
419d327
9e9e117
0da4b8f
c470fb3
c44d321
537fa37
14bae20
b951127
9d035f5
c31c191
0872e6a
de92ac5
0e6cea6
8afdd9b
7018f72
d81ac52
332576e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I don't think a feature flag for this is needed. We can just state in the docs (about-versioning.md) that the endpoint is experimental.
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.
OK, done.
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.
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.
Agreed, fixed in next push.
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.
This function lives in
pkg/distributor
, I would say it's a little bit pretentious to take the nameparser
for this :DHow about
influxRequestParser
?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.
Agreed, fixed in next push.
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.
Opinionated style nit: I don't think we need 4 lines for this debug log.
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.
Fixed in next push.
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.
Does this logger have all the required context? Can you also add some context here about what was going on when this happened? I'm scared of finding this log:
Also, nit,
.Error()
call is not needed, just passerr
.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.
Agreed, fixed in next push.
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.
You should use
mimirpb.PreallocTimeseriesSliceFromPool()
instead of creating a new slice every time.Also, IMO it would make sense to change
influxpush.ParseInfluxLineReader
to return[]PreallocTimeseries
instead of[]Timesries
because that's what we deal with later.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.
Done. Fixed in latest push.
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.
I don't see any httpgrpc errors being returned by
parser
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.
Oops, should have got rid of that one. Will resolve once I work out best way to wrap existing error in the StatusBadRequest.
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.
Ah,
httpgprc
is used to smuggle both the http status code and the error message out of thesupplier()
function. I've removed the misleading comment.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.
This would definitely help customer to debug.
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.
Done. Bubbled
bytesRead
down from the parsing function so that it is available for both metrics/histograms and for logging here.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.
While the code below isn't extremely complex, it's still a couple of
if
s, so I'd write here some sane defaults.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.
Agreed. Done!
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.
Had to add the var definitions back in as the linter was complaining that the initial assignments/defaults were unused.
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.
Logs with
insight=true
are visible to our Grafana Cloud customers.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.
Aha, TIL! Will add it back in as that could be useful.
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.
Why do we need to
tryUnwrap
? Wrapping errors provides details about what went wrong, this is literally "tryToRemoveDetails".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.
Legacy from the original code: https://github.com/grafana/influx2cortex/blob/main/pkg/influx/errors.go
Removed now.
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.
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.
Done.
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.
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.
Done.