Skip to content
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

adds the ability to write the generated series into a file #8

Closed
wants to merge 1 commit into from

Conversation

replay
Copy link
Contributor

@replay replay commented Nov 11, 2021

This is useful to validate that our test definition is generating the series we think it should be generating.

When running like this:

K6_PROMETHEUS_REMOTE_WRITE_DEBUG=/tmp/debug1 ./k6 run ./test.js

It will write the generated series into /tmp/debug1.

Comment on lines +38 to +41
for _, envVar := range os.Environ() {
if len(envVar) > len(debugVar)+1 && envVar[:len(debugVar)+1] == debugVar+"=" {
debugFile := envVar[len(debugVar)+1:]
debugWriter = &LockedWriter{file: debugFile}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use os.LookupEnv

Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the requested changes are blocking this IMO.

On the other hand I don't think this change is all that interesting/useful for normal usage of the extension, so I would probably want it removed when we make this more ... presentable. But even if we don't I kind of would prefer if most of this (especially with some of the proposed changes) be moved in a separate file and interfere as little as possible with the rest of the extension.

Again though we can do that later :)

Comment on lines +56 to +59
f, err := os.OpenFile(lw.file, os.O_APPEND|os.O_WRONLY|os.O_CREATE, 0600)
if err != nil {
return 0, err
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do feel like it will be better if we open it once on initializaiton

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that makes it harder to figure out when to close it, but I doubt that is a problem, I am pretty sure that os.File doesn't have any buffering by default so everything will be in kernel buffers either way so 🤷

Comment on lines +393 to 408
if debugBuilder != nil {
debugBuilder.WriteString(fmt.Sprintf(
"series: %s value: %f timestamp: %d\n",
labelsToString(series[seriesID-minSeriesID].Labels),
series[seriesID-minSeriesID].Samples[0].Value,
series[seriesID-minSeriesID].Samples[0].Timestamp,
))
}
}

if debugWriter != nil && debugBuilder != nil {
_, err := debugWriter.Write(debugBuilder.String())
if err != nil {
log.Fatal(err)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe all of this can just be done over the returned []prompb.TimeSeries possibly even not in this function but some of the others where it's also send maybe in store itself through a single dumpTimeSeriesInDebugFile call

@jdbaldry
Copy link
Member

jdbaldry commented Jan 4, 2022

@replay Sorry for the delay, are you still looking for a review on this?

@replay
Copy link
Contributor Author

replay commented Jan 4, 2022

@replay Sorry for the delay, are you still looking for a review on this?

Thanks for asking, i think that's not necessary anymore :) I'll close the PR

@replay replay closed this Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants