-
Notifications
You must be signed in to change notification settings - Fork 104
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
Use go modules and implement additional remote write flags. #8
Use go modules and implement additional remote write flags. #8
Conversation
9e12867
to
8ad614d
Compare
Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>
8ad614d
to
c4d8749
Compare
Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>
3743ce5
to
96849d6
Compare
Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>
96849d6
to
a92ed6c
Compare
Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>
@krasi-georgiev are we planning to merge this ? |
That is the idea, just waiting on some feedback from the maintainers. Would that change be useful to you as well? |
yes, I am really hoping to use -remote-url asap |
I was hoping to get to this already but this week has gotten very busy :( Hopefully I will get to it this weekend! |
Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>
Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>
Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>
} | ||
}() | ||
|
||
go func() { | ||
for tick := range seriesTick.C { | ||
fmt.Printf("%v: refreshing series cycle\n", tick) | ||
metricsMux.Lock() |
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.
moved the lock here because otherwise there is a race for seriesCycle++
causing cycleValues
to misbehave
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.
Did a first pass, I with some comments, I will plan to do another pass/some testing when I get a chance. A couple general comments:
- It would be nice to break some of the pprof code into separate functions so that the flow of the main code reads better. Need to think a bit on the best way to do this.
- I like that the requests are sent in parallel for each batch, thanks for making sure of that.
- What do you think of keeping
vendor/
or not?
go.mod
Outdated
github.com/prometheus/client_golang v1.0.0 | ||
github.com/prometheus/client_model v0.0.0-20190129233127-fd36f4220a90 | ||
github.com/prometheus/common v0.4.1 | ||
github.com/prometheus/prometheus v0.0.0-20190302143042-82f98c825a14 |
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 think it would be better to pin to a Prometheus release.
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.
wasted few hours trying to ping it to v2.11.1 but go mod tidy
keeps reverting it to the untagged version although the commit id is the same as v2.11.1
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.
Well, that's silly. Thanks for trying.
metrics/write.go
Outdated
merr = &errors.MultiError{} | ||
) | ||
|
||
fmt.Printf("Sending: %v timeseries, %v samples, %v timeseries per request, %v delay between requests\n", len(tss), c.config.SamplesCount, c.config.BatchSize, c.config.RequestInterval) |
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 would vote to consistently use log
rather than fmt
.
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.
updated
end = len(tss) | ||
} | ||
req := &prompb.WriteRequest{ | ||
Timeseries: tss[i:end], |
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.
Are the timestamps of the samples being updated anywhere? If not they should be updated for each send otherwise systems like Cortex will reject the samples as duplicates.
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.
the timestamp is set here
https://github.com/open-fresh/avalanche/pull/8/files#diff-a3417332ad87f5118a22dec21e5038b5L99
So this PR doesn't change this behavior.
this means that when sending the batches the timestamp would be the same for concurrent requests, but these are samples for different timeseries anyway.
In other words something like:
batch1
request1
series1 timestamp1
request2
series2 timestamp1
batch2
request1
series1 timestamp2
request2
series2 timestamp2
would that be a problem?
the only difference is that now requests are sent in parallel, where before they were sent in serial.
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.
oooh , no actually I just noticed that it doesn't update the timestamps, will think how to fix it 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.
updated, the workflow is now as per my first comment
let me know if this would work.
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 think this looks good now!
8c4c604
to
dd3713c
Compare
Yes I think the same and I tried few things as well, but each one hit a blocker so decided to keep it as is and will think how to improve later when/if needed.
I don't like that it extends the time required to build the docker image significantly, but I thought that vendor-ed dirs would eventually need to go, but I don't feel strongly about it. |
Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>
I just had an idea that we can mount the host mod cach dir and this way building the docker image would not need to download anything |
Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>
I improved the Dockerfile and now it caches the dependencies in a separate step so that it re-downloads everything only when go.mod/do.sum changes. Relevant discussions in microsoft/vscode-go#1945 |
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 like the dockerfile update, that will be nice. Left a couple of last comments, then I think this will be good to go.
end = len(tss) | ||
} | ||
req := &prompb.WriteRequest{ | ||
Timeseries: tss[i:end], |
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 think this looks good 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.
Thanks!
Is there a docker image for this and can you update it with this change? |
quay.io/freshtracks.io/avalanche should have been updated when I merged this. Latest tag, or the tag with the appropriate commit sha suffix will work. Let me know if it doesn’t and I will fix the CI! |
I think the readme should mention the profiling capability. |
…us-community#8) * Use go modules and implement remote write samples count flag. Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * concurent send Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * added continious profiling. Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * fix dockerfile Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * fix remotePprof wait Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * fix the series update lock race Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * total samples count Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * nits Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * update ts for each request Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * improve the cache for docker build Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> * nits Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>
it send the requests for each series batch in parallel
For each sample it makes totalSeries/seriesPerRequest and it doesn't continue to the next sample until the current series are sent.
Using --remote-send-batch one can configure how many requests are sent in parallel.
For example if you want to send 10 requests in parallel the whole example would would look like:
Also added continuous or a single pprof
For reference here is a more complete readme with examples:
https://github.com/krasi-georgiev/benchTSDB
Maybe I can even add these examples from my repo here.
Signed-off-by: Krasi Georgiev 8903888+krasi-georgiev@users.noreply.github.com