-
Notifications
You must be signed in to change notification settings - Fork 453
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] Docker integration tests for prom-remote storage #3768
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3768 +/- ##
======================================
Coverage 57.1% 57.1%
======================================
Files 552 552
Lines 62984 62984
======================================
Hits 35974 35974
Misses 23819 23819
Partials 3191 3191
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
cc92307
to
5687793
Compare
success=$(echo "$out" | grep -v promremotecli_log | docker run --rm -i "$JQ_IMAGE" jq .success) | ||
status=$(echo "$out" | grep -v promremotecli_log | docker run --rm -i "$JQ_IMAGE" jq .statusCode) |
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 are these being piped though jq docker container? Could it pipe through local jq instead (i.e.... | jq ...
) like in other functions in this file?
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 am not sure. My guess - to not force installation on JQ on a running machine. I copied (and removed some uneeded stuff) from other tests.
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.
That seems reasonable. However, IIUC, other code in the same file uses local jq, e.g.
ATTEMPTS=50 TIMEOUT=2 MAX_TIMEOUT=4 retry_with_backoff \ | |
"[[ \$(curl -sSf $host/api/v1/query?query=$metric_name | jq -r .data.result[0].value[1]) -gt 0 ]]" |
So we still have to have jq on machine, with the additional complexity of running some of the jq commands through docker container.
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 yeah, actually I used local myself in couple of commands without thinking. Will use local here as well.
What this PR does / why we need it:
In this PR docker integration test suite is implemented to cover Prom Remote Write backend use case from #3742.
Special notes for your reviewer:
For tests we setup:
--enable-feature=remote-write-receiver
Coordinator has 2 endpoints setup
prometheusraw
andprometheusagg
. Metrics are written to coordinator using remote write APIhttp://m3coordinator01:7201/api/v1/prom/remote/write
. There is a rollup rule that gets applied and metric gets renamed before sending to aggregated prometheus.Both prometheus are queried and are expected to return a metric.
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: