-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix: correctly handle __raw_url__
by replacing url with it if present
#72
Conversation
8a92a01
to
f92bd13
Compare
f92bd13
to
00637f5
Compare
afc8a18
to
d7c3541
Compare
Still testing this in dev, but should be reviewable in the meantime. |
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
ctx, cancel := context.WithTimeout(context.Background(), time.Minute) | ||
t.Cleanup(cancel) | ||
|
||
outFile := filepath.Join(t.TempDir(), "metrics.txt") | ||
|
||
cmd := exec.CommandContext(ctx, smk6, "run", "-", "-o=sm="+outFile) | ||
cmd.Stdin = bytes.NewReader(testScript) | ||
err = cmd.Run() | ||
if err != nil { | ||
t.Fatalf("running sm-k6: %v", err) | ||
if k6out, err := cmd.CombinedOutput(); err != nil { | ||
t.Fatalf("running sm-k6: %v\n%s", errors.Join(err, ctx.Err()), string(k6out)) |
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 struggled to reliably get tests to pass without this. I'll probably get rid of these changes in favor of #73, when and if it gets merged.
Sorry for tagging you directly @d0ugal, but I think you now have a decent hunk of context for this. Hopefully this is a simple change to review! |
This PR introduced special handling for the
__raw_url__
tag, which matches what the agent would expect as per grafana/synthetic-monitoring-agent#683.It also adds tests to assert that the label is correclty replaced, and that
__raw_url__
is removed from the output, as otherwise it would make the output invalid as per the prometheus spec.