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

Expose profiling endpoints #3370

Merged
merged 1 commit into from
Nov 8, 2023
Merged

Expose profiling endpoints #3370

merged 1 commit into from
Nov 8, 2023

Conversation

olegbespalov
Copy link
Contributor

@olegbespalov olegbespalov commented Oct 5, 2023

What?

This change exposes profiling endpoints for the k6.

Why?

This won't have significant performance implications (if the endpoints are not used), so we could expose them by default.

But having this endpoint allows efficient development of k6 (and extension)

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make ci-like-lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

Closes #1692

@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2023

Codecov Report

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

Comparison is base (e44a08f) 73.36% compared to head (104862f) 73.36%.

❗ Current head 104862f differs from pull request most recent head a43edef. Consider uploading reports for the commit a43edef to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3370   +/-   ##
=======================================
  Coverage   73.36%   73.36%           
=======================================
  Files         261      261           
  Lines       19742    19760   +18     
=======================================
+ Hits        14483    14497   +14     
- Misses       4365     4367    +2     
- Partials      894      896    +2     
Flag Coverage Δ
ubuntu 73.23% <82.60%> (-0.07%) ⬇️
windows 73.19% <82.60%> (+<0.01%) ⬆️

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

Files Coverage Δ
cmd/root.go 94.23% <100.00%> (+0.23%) ⬆️
cmd/run.go 74.91% <100.00%> (+0.63%) ⬆️
cmd/state/state.go 6.94% <100.00%> (+1.31%) ⬆️
api/server.go 76.00% <33.33%> (-6.61%) ⬇️
cmd/ui.go 69.65% <0.00%> (-0.71%) ⬇️

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

mstoykov
mstoykov previously approved these changes Oct 17, 2023
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.

Not certain if we shouldn't make this configruable but otherwise LGTM!

My predominant problem wiht lack of configuration isn't even it being "expensive" to have it around - it isn't.

But the fact that we add it on top of the /api/v1 endpoint which we might drop at some future point and then anything using this needs to change. But I feel like it will probably be better at taht future point to leave this endpoint where it is and build v2 around it 🤷

codebien
codebien previously approved these changes Oct 17, 2023
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.

LGTM

oleiade
oleiade previously approved these changes Oct 18, 2023
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.

👏🏻

@olegbespalov
Copy link
Contributor Author

olegbespalov commented Oct 20, 2023

I am also unsure if we need to expose the profiling endpoints by default even if it doesn't harm (at least, I can't name the cases). It also aligns with other grafana's projects where enabling profiles are usually controlled by setting. So, if the team decides, I'll happily produce one more commit.

@oleiade @codebien, anybody else who sees that?

Please vote on this comment:

  • 👍 let's add the config option
  • 👎 no need to option

@oleiade
Copy link
Member

oleiade commented Oct 20, 2023

I would align with the Grafana ecosystem and have config for it 👍🏻

@codebien
Copy link
Contributor

grafana's projects where enabling profiles are usually controlled by setting

even if it doesn't harm (at least, I can't name the cases).

Maybe we can find more rationale about why in their original issues.

@olegbespalov olegbespalov dismissed stale reviews from oleiade, codebien, and mstoykov via 66dc9c5 November 7, 2023 16:03
@olegbespalov olegbespalov added the documentation-needed A PR which will need a separate PR for documentation label Nov 7, 2023
@olegbespalov
Copy link
Contributor Author

Hey there! I've implemented a flag --profile-enabled (it's pretty close to the one I saw in other grafana products).

Unfortunately, it's not so easy to add the configuration via environment variables; for example, REST API has no way to configure it through environment variables. But for the starter, that should be enough.

oleiade
oleiade previously approved these changes Nov 7, 2023
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 🚀

As I tried the feature locally, I had to go and read the code to find out what port to use in the URL.

I wonder if we could somehow display the profile URL as part of k6 output somehow. The dashboard project displays it as part of the output line at the moment. Feel free to merge without, and don't consider my comment blocking 👍🏻

Screenshot 2023-11-07 at 17 58 27

mstoykov
mstoykov previously approved these changes Nov 8, 2023
cmd/run.go Outdated
go func() {
defer apiWG.Done()
logger.Debugf("Starting the REST API server on %s", c.gs.Flags.Address)
logger.Debugf("Profiling endpoints exposed: %v", c.gs.Flags.ProfilingEnabled)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer this to be behind an if instead of printing a false/true

@olegbespalov olegbespalov dismissed stale reviews from mstoykov and oleiade via 2089d5b November 8, 2023 09:05
@olegbespalov
Copy link
Contributor Author

@oleiade @mstoykov I've tried to combine both of your suggestions under the 2089d5b

image

Would this make sense?

mstoykov
mstoykov previously approved these changes Nov 8, 2023
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.

LGTM!

I still kind of prefer to have a debug log as well, but I am okay either way.

@olegbespalov
Copy link
Contributor Author

@mstoykov no problem, I'll adjust and add log 👍

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.

🚀 🚀 🚀

@olegbespalov olegbespalov added this to the v0.48.0 milestone Nov 8, 2023
@olegbespalov olegbespalov merged commit 0db029a into master Nov 8, 2023
21 checks passed
@olegbespalov olegbespalov deleted the feat/pprof branch November 8, 2023 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation-needed A PR which will need a separate PR for documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add profiling endpoint
5 participants