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: enable skaffold render to track telemetry #9020

Merged

Conversation

renzodavid9
Copy link
Contributor

@renzodavid9 renzodavid9 commented Aug 14, 2023

Related: #9013

Description
This PR configures the skaffold render command to start tracking telemetry data when it is used. Currently the logic for emitting metrics is related to the logic for displaying the Skaffold survey URL, the opt-out message, and the version upgrade. These messages were corrupting the output when used, e.g, like skaffold render > manifest.yaml, due to everything was printed using stdout. Now we are changing from stdout to stderr to print those messages.

@renzodavid9 renzodavid9 changed the title Tracking render command feat: enable skaffold render to track telemetry Aug 15, 2023
@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Merging #9020 (e03607d) into main (290280e) will decrease coverage by 6.86%.
Report is 1004 commits behind head on main.
The diff coverage is 49.90%.

@@            Coverage Diff             @@
##             main    #9020      +/-   ##
==========================================
- Coverage   70.48%   63.62%   -6.86%     
==========================================
  Files         515      624     +109     
  Lines       23150    31934    +8784     
==========================================
+ Hits        16317    20318    +4001     
- Misses       5776    10084    +4308     
- Partials     1057     1532     +475     
Files Changed Coverage Δ
cmd/skaffold/app/cmd/completion.go 13.04% <0.00%> (-1.25%) ⬇️
cmd/skaffold/app/cmd/config/list.go 65.21% <ø> (ø)
cmd/skaffold/app/cmd/config/set.go 88.72% <ø> (ø)
cmd/skaffold/app/cmd/config/util.go 54.28% <ø> (ø)
cmd/skaffold/app/cmd/credits.go 100.00% <ø> (ø)
cmd/skaffold/app/cmd/credits/export.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/deploy.go 40.90% <0.00%> (-12.94%) ⬇️
cmd/skaffold/app/cmd/generate_pipeline.go 60.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_modules.go 65.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_profiles.go 66.66% <ø> (ø)
... and 40 more

... and 419 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pull-request-size pull-request-size bot added size/L and removed size/M labels Aug 17, 2023
@@ -53,9 +52,6 @@ func ShouldDisplayMetricsPrompt(configfile string) bool {
}

func DisplayMetricsPrompt(configFile string, out io.Writer) error {
if isStdOut(out) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was checking the idea behind this condition, it was checking if the given io.Writer was a stdout, but from my understanding in the code, we were always sending an stdout, so it was always true, and I didn't see other place where we were using this function or sending other type of io.Writer, so I removed the condition + function completely 😅 . Let me know if you see a case I'm not taking into account where this condition can be false.

@renzodavid9 renzodavid9 marked this pull request as ready for review August 17, 2023 15:39
@renzodavid9 renzodavid9 merged commit 2ba7d48 into GoogleContainerTools:main Aug 17, 2023
15 checks passed
renzodavid9 added a commit that referenced this pull request Sep 7, 2023
* feat: making survey, update, and tracking messages to print to stderr instead of stdout

* feat: enable `skaffold render` to track telemetry

* feat: remove tests checking if a stadout is set, now we are using stderr

* feat: make stderr used to print survey, update, and metricts promp, coloreable to keep same behaviour as stdin

* feat: removing IsStdout function and related tests
renzodavid9 added a commit that referenced this pull request Sep 8, 2023
* feat: configure verify and exec commands to emit metrics (#9013)

* feat: configure render, verify, and exec commands to emit metrics

* fix: remove render command from tracking due to corrupted output data

* feat: enable skaffold render to track telemetry (#9020)

* feat: making survey, update, and tracking messages to print to stderr instead of stdout

* feat: enable `skaffold render` to track telemetry

* feat: remove tests checking if a stadout is set, now we are using stderr

* feat: make stderr used to print survey, update, and metricts promp, coloreable to keep same behaviour as stdin

* feat: removing IsStdout function and related tests

* fix: change baseRef to point to v2.6 release
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants