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

feat: Integrating the web dashboard feature #3505

Merged
merged 30 commits into from
Jan 16, 2024

Conversation

szkiba
Copy link
Contributor

@szkiba szkiba commented Dec 15, 2023

As per our product DNA, we aim, by integrating the web dashboard (xk6-dashboard) directly into k6, to enhance and streamline the user experience, making real-time and end-of-test visualizations immediately accessible as part of the k6 command-line experience.

This is not yet the final version of the integration. The goal is to integrate xk6-dashboard v0.7.0, but that release does not exist yet (waiting for dependencies).

I created the PR to ask for a review and to get feedback on whether the integration would be good this way, or if the team had something else in mind.

As per our product DNA, we aim, by integrating the web dashboard (xk6-dashboard) directly into k6, to enhance and
streamline the user experience, making real-time and end-of-test visualizations immediately accessible as part of the k6
command-line experience.
@szkiba szkiba linked an issue Dec 15, 2023 that may be closed by this pull request
@CLAassistant
Copy link

CLAassistant commented Dec 15, 2023

CLA assistant check
All committers have signed the CLA.

go.mod Outdated Show resolved Hide resolved
Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

Hey @szkiba,
thanks for your contribution. Congrats for the progress with the dashboard 👏

I've left some comments.

cmd/config.go Outdated Show resolved Hide resolved
cmd/outputs.go Outdated Show resolved Hide resolved
cmd/outputs.go Outdated Show resolved Hide resolved
cmd/ui.go Outdated Show resolved Hide resolved
cmd/ui.go Outdated Show resolved Hide resolved
@szkiba
Copy link
Contributor Author

szkiba commented Dec 15, 2023

Hey @szkiba, thanks for your contribution. Congrats for the progress with the dashboard 👏

I've left some comments.

Hi @codebien , thanks for the feedback!

@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (d00e4f6) 72.96% compared to head (da52b66) 72.96%.
Report is 12 commits behind head on master.

❗ Current head da52b66 differs from pull request most recent head 7f43e50. Consider uploading reports for the commit 7f43e50 to get more accurate results

Files Patch % Lines
cmd/ui.go 91.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3505   +/-   ##
=======================================
  Coverage   72.96%   72.96%           
=======================================
  Files         280      280           
  Lines       20938    20949   +11     
=======================================
+ Hits        15277    15286    +9     
- Misses       4691     4693    +2     
  Partials      970      970           
Flag Coverage Δ
macos 72.89% <95.45%> (?)
ubuntu 72.91% <95.45%> (-0.01%) ⬇️
windows 72.81% <95.45%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@szkiba szkiba requested review from codebien and mstoykov December 18, 2023 08:24
@szkiba szkiba requested a review from a team as a code owner December 18, 2023 13:17
Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Will wait until the rest of the environment variables are implemented, and we resolve some of the discussions we're having internally to approve. But looks good to me, great job @szkiba 👏🏻

@szkiba
Copy link
Contributor Author

szkiba commented Dec 20, 2023

LGTM 🚀

Will wait until the rest of the environment variables are implemented, and we resolve some of the discussions we're having internally to approve. But looks good to me, great job @szkiba 👏🏻

Thank you @oleiade

What environment variables are you referring to?
Here is the list of environment variables managed by xk6-dashboard:

  • K6_WEB_DASHBOARD_HOST
  • K6_WEB_DASHBOARD_PORT
  • K6_WEB_DASHBOARD_PERIOD
  • K6_WEB_DASHBOARD_OPEN
  • K6_WEB_DASHBOARD_EXPORT
  • K6_WEB_DASHBOARD_RECORD
  • K6_WEB_DASHBOARD_TAG

see: https://github.com/grafana/xk6-dashboard#environment

In addition, the K6_WEB_DASHBOARD variable can be set to true to activate the dashboard in k6.
Correct me if I'm wrong, but I think all environment variables in the design document are implemented....

see: https://github.com/grafana/k6/blob/feat/3504-dashboard-integration/cmd/config.go#L45

@oleiade
Copy link
Member

oleiade commented Dec 20, 2023

This is my bad @szkiba, I came in with the wrong assumption. You're right, they should likely remain defined in the extension itself 👍🏻

.gitignore Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
@oleiade oleiade requested review from codebien and oleiade January 15, 2024 09:23
@mstoykov mstoykov added this to the v0.49.0 milestone Jan 15, 2024
olegbespalov
olegbespalov previously approved these changes Jan 15, 2024
cmd/config.go Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
cmd/tests/cmd_run_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

The current code looks good to me.

But, unfortunately, there is something in the UX that sounds unexpected.
If I send an interrupt (CTRL+c) while k6 is running and the dashboard is opened in the browser then k6 gets stuck.

@szkiba is it expected?

I tested running the following command:

K6_WEB_DASHBOARD=true go run . run -d 1m ./examples/http_get.js

@szkiba
Copy link
Contributor Author

szkiba commented Jan 16, 2024

The current code looks good to me.

But, unfortunately, there is something in the UX that sounds unexpected. If I send an interrupt (CTRL+c) while k6 is running and the dashboard is opened in the browser then k6 gets stuck.

@szkiba is it expected?

I tested running the following command:

K6_WEB_DASHBOARD=true go run . run -d 1m ./examples/http_get.js

@codebien Actually, this is a feature (as long as the browser is open, k6 does not stop after the test run), but it really does not handle the abort situation well. I will fix it and make a patch release soon.

update: done, v0.7.1 handle the test abort situation. xk6-dashboard dependency upgraded to v0.7.1
FYI @codebien @oleiade

Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

The interrupt logic it's now fixed for me.

@codebien codebien requested a review from mstoykov January 16, 2024 09:18
Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

🚀

@oleiade oleiade requested a review from olegbespalov January 16, 2024 09:43
@oleiade oleiade merged commit be8527a into master Jan 16, 2024
24 checks passed
@oleiade oleiade deleted the feat/3504-dashboard-integration branch January 16, 2024 12:58
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.

Integrating the web dashboard feature into k6
7 participants