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

Add flag to save events to a file #5125

Merged

Conversation

priyawadhwa
Copy link
Contributor

Adds the --event-log-file flag, which when set to a filepath will save Skaffold events to a file when --enable-rpc is also enabled.

This will make it easier to collect data around performance metrics for the performance dashboards. We can just parse the events in the file after skaffold executes, and figure out how long each stage took.

Adds the --event-log-file flag, which when set to a filepath will save Skaffold events to a file when `--enable-rpc` is also enabled.
@priyawadhwa priyawadhwa requested a review from a team as a code owner December 9, 2020 02:20
@google-cla google-cla bot added the cla: yes label Dec 9, 2020
@MarlonGamez
Copy link
Contributor

This looks good, It looks like jsonpb is deprecated in favor of protojson though

@priyawadhwa
Copy link
Contributor Author

priyawadhwa commented Dec 9, 2020

Yah I noticed that too -- unfortunately it isn't as simple as just changing the name of the import, since it looks like the upgrade isn't backwards compatible. Replacing with protojson fails with this compiler error:

cannot use &logEntry (value of type *proto.LogEntry) as protoreflect.ProtoMessage value in argument to protojson.Unmarshal: missing method ProtoReflect

I think if we want the upgraded library to work we also have to recreate our proto file with a new version of protoc-gen-go built from the new library. This would require:

  • Changing all of our files that require jsonpb to use protojson (not too hard)
  • Changing the Dockerfile used for upgrading/testing the proto to use protojson and upgrade protoc-gen-go to match (feel like this is trickier than it sounds)
  • Making sure the newly generated proto is backwards compatible with existing users/IDEs

Until we do that big upgrade, I think we have to keep using the deprecated version :/ I guess I'll have to see if there's a way around it for the unit test.

@codecov
Copy link

codecov bot commented Dec 9, 2020

Codecov Report

Merging #5125 (d61ca86) into master (53a81ad) will decrease coverage by 0.00%.
The diff coverage is 41.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5125      +/-   ##
==========================================
- Coverage   72.14%   72.13%   -0.01%     
==========================================
  Files         381      381              
  Lines       13474    13491      +17     
==========================================
+ Hits         9721     9732      +11     
- Misses       3042     3046       +4     
- Partials      711      713       +2     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/flags.go 82.85% <ø> (ø)
pkg/skaffold/config/options.go 100.00% <ø> (ø)
pkg/skaffold/server/server.go 43.56% <0.00%> (-1.80%) ⬇️
pkg/skaffold/event/event.go 90.76% <53.84%> (-1.15%) ⬇️
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 53a81ad...d61ca86. Read the comment docs.

Copy link
Contributor

@MarlonGamez MarlonGamez left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for clarifying the proto situation 👍🏼

@MarlonGamez MarlonGamez merged commit 98321e4 into GoogleContainerTools:master Dec 9, 2020
@priyawadhwa priyawadhwa deleted the log-events-flag branch December 9, 2020 22:36
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.

2 participants