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

working cloud profiler export for skaffold #6066

Merged

Conversation

aaron-prindle
Copy link
Contributor

@aaron-prindle aaron-prindle commented Jun 22, 2021

Description

Adds cloud profiler code to skaffold that is able to upload profile information to GCP's Cloud Profiler using user's own gcloud application default credentials.

Alternatives

This could be done as a build tag and not compiled into skaffold binary but the upside is not worth it becuause:

  • The binary size difference is negligile
    • skaffold master branch @ HEAD
      • aprindle@aprindle ~/skaffold  [master]$ ls -lh out/skaffold
        -rwxr-x--- 1 aprindle primarygroup 61M Jun 22 14:52 out/skaffold
        
    • skaffold fix-5789_cloud-profiler @ HEAD (this PR)
      • aprindle@aprindle ~/skaffold  [master]$ ls -lh out/skaffold
        -rwxr-x--- 1 aprindle primarygroup 62M Jun 21 21:46
        
  • make linters checks would require adding tweaks/hacks as they do not support parsing two of the same function currently as build tags would require

Considerations

Similar to SKAFFOLD_TRACE, the SKAFFOLD_PROFILE functionality will be used in cron tests around skaffold's performance, do not affect skaffold's performance unless enabled (as would be expected) and add negligible size to the skaffold binary (~61M->~62M)

@aaron-prindle aaron-prindle requested a review from a team as a code owner June 22, 2021 21:59
@aaron-prindle aaron-prindle requested a review from tejal29 June 22, 2021 21:59
@google-cla google-cla bot added the cla: yes label Jun 22, 2021
@aaron-prindle aaron-prindle force-pushed the fix-5789_cloud-profiler branch from 7e5c35a to 68c4138 Compare June 22, 2021 22:00
@codecov
Copy link

codecov bot commented Jun 22, 2021

Codecov Report

Merging #6066 (652cd70) into master (01bdaaf) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6066      +/-   ##
==========================================
- Coverage   69.32%   69.30%   -0.02%     
==========================================
  Files         481      481              
  Lines       18478    18490      +12     
==========================================
+ Hits        12810    12815       +5     
- Misses       4713     4723      +10     
+ Partials      955      952       -3     
Impacted Files Coverage Δ
cmd/skaffold/skaffold.go 0.00% <0.00%> (ø)
pkg/skaffold/docker/parse.go 86.19% <0.00%> (-0.96%) ⬇️
pkg/skaffold/docker/image.go 79.53% <0.00%> (+1.39%) ⬆️
pkg/skaffold/util/tar.go 56.00% <0.00%> (+5.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01bdaaf...652cd70. Read the comment docs.

@aaron-prindle aaron-prindle force-pushed the fix-5789_cloud-profiler branch from 68c4138 to 19bc581 Compare June 23, 2021 04:57
@tejal29
Copy link
Contributor

tejal29 commented Jun 23, 2021

Similar to SKAFFOLD_TRACE, the SKAFFOLD_PROFILE functionality will be used in cron tests around skaffold's performance, do not affect skaffold's performance unless enabled (as would be expected) and add negligible size to the skaffold binary

SKAFFOLD_PROFILE is already used for --profile flag. Can we change this name to SKAFFOLD_PROFILER ?

@tejal29 tejal29 self-assigned this Jun 23, 2021
Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

1 nit and more explanation.

)

type ExitCoder interface {
ExitCode() int
}

func main() {
if _, ok := os.LookupEnv("SKAFFOLD_PROFILE"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if _, ok := os.LookupEnv("SKAFFOLD_PROFILE"); ok {
if _, ok := os.LookupEnv("SKAFFOLD_PROFILER"); ok {

See here https://skaffold.dev/docs/references/cli/#skaffold-apply ( search for SKAFFOLD_PROFILE)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed s/SKAFFOLD_PROFILE/SKAFFOLD_PROFILER

)

type ExitCoder interface {
ExitCode() int
}

func main() {
if _, ok := os.LookupEnv("SKAFFOLD_PROFILE"); ok {
err := profiler.Start(profiler.Config{
Service: "skaffold_examples_microservices",
Copy link
Contributor

Choose a reason for hiding this comment

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

What does is service mean here? Sorry I don't have context on cloud.google.com/go/profiler?
Shd this service be deployed in the project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to be configurable via env var - SKAFFOLD_PROFILER_SERVICE

@aaron-prindle aaron-prindle force-pushed the fix-5789_cloud-profiler branch 3 times, most recently from e718646 to f20ecaa Compare July 7, 2021 17:24
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jul 7, 2021
@aaron-prindle aaron-prindle force-pushed the fix-5789_cloud-profiler branch from f20ecaa to ecf91f9 Compare July 7, 2021 21:07
@pull-request-size pull-request-size bot added size/S and removed size/M labels Jul 7, 2021
@tejal29 tejal29 enabled auto-merge (squash) July 7, 2021 21:42
@aaron-prindle aaron-prindle force-pushed the fix-5789_cloud-profiler branch 2 times, most recently from a2349a8 to e8ff2f2 Compare July 7, 2021 22:57
@aaron-prindle aaron-prindle force-pushed the fix-5789_cloud-profiler branch from e8ff2f2 to 652cd70 Compare July 8, 2021 18:18
@aaron-prindle aaron-prindle added the kokoro:force-run forces a kokoro re-run on a PR label Jul 8, 2021
@kokoro-team kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Jul 8, 2021
@tejal29 tejal29 merged commit c2b6deb into GoogleContainerTools:master Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants