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

Upgrade to v0.53 and support with-browser #29

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anton-iermolenko
Copy link

Adds support for usage of k6 timescaledb with browser-based load testing. Adds modified dashboard sample tuned for k6 browser metrics

Adds support for usage of k6 timescaledb with browser-based load testing. Adds modified dashboard sample tuned for k6 browser metrics
@CLAassistant
Copy link

CLAassistant commented Aug 25, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@andrewslotin andrewslotin self-requested a review August 27, 2024 14:33
@oleghalin
Copy link

oleghalin commented Oct 15, 2024

Any updates on PR review?

@andrewslotin
Copy link

Hey, @anton-iermolenko! Thanks a lot for your contribution and apologies for the late answer. Could you elaborate on your use case and explain why these changes are needed?

@oleghalin
Copy link

Without this changes it's impossible to use timescale plugin with latest k6

K6 built on fixed version passed in gomod of this plugin

@anton-iermolenko
Copy link
Author

Hey, @anton-iermolenko! Thanks a lot for your contribution and apologies for the late answer. Could you elaborate on your use case and explain why these changes are needed?

I needed to use K6 for browser-based load testing on multiple nodes with output into some storage. Starting k6 0.52.0 browser module is bundled by default, timeseries plugin not. Without these changes you cannot cook k6 which supports both. Dashboard sample added because latest version of k6 in browser mode spits Core Web Vitals differently into DB, so I had to rewrite queries pulling data from it to visualize in Grafana.

@andrewslotin
Copy link

@oleghalin, @anton-iermolenko, I see and upgrading to the latest k6 makes total sense. However, I'm not sure about adding the browser and Grafana dashboards though. We would prefer to keep extension code focused on providing the intended functionality, i.e. writing metrics to the TimescaleDB in this case, and avoid adding new use cases. You are more than welcome though to use grafana/xk6 as a image and add browser there.

Copy link

@andrewslotin andrewslotin left a comment

Choose a reason for hiding this comment

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

As per the comment above, I'd suggest to reduce these changes to upgrading the k6 version and move the dashboard and Dockerfile changes outside of the PR scope.

WORKDIR $GOPATH/src/go.k6.io/k6
ADD . .
RUN apk --no-cache add git
RUN CGO_ENABLED=0 go install go.k6.io/xk6/cmd/xk6@latest
RUN CGO_ENABLED=0 xk6 build --with github.com/grafana/xk6-output-timescaledb=. --output /tmp/k6
RUN CGO_ENABLED=0 xk6 build v0.53.0 --with github.com/grafana/xk6-output-timescaledb=. --output /usr/bin/k6

Choose a reason for hiding this comment

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

By default xk6 uses the latest version of k6 required by extensions and since xk6-output-timescaledb is now locked to v0.53, this will be the lowest version used to build the binary. Is there any other reason to v0.53 specified here explicitly? Otherwise I'd suggest to drop this change.

Choose a reason for hiding this comment

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

Also any reason to change the target directory to /usr/bin?

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.

4 participants