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

Embed metrics credentials and upload metrics if they are present #5157

Merged
merged 3 commits into from
Dec 17, 2020

Conversation

IsaacPD
Copy link
Contributor

@IsaacPD IsaacPD commented Dec 16, 2020

Description

  • Modify generate-statik.sh to look for a secret directory and create a statik package for it.
  • Add dependency to OpenTelemetry and GoogleCloudPlatform/opentelemtry-operations-go.
  • Count the number of times artifact types appear to track which builders have the most artifacts
  • Determine if the skaffold user is running it online by running a function in a go-routine to update a global variable
    • If it does not finish in time, it will get written to a file until the next run where there is enough time to check if it is online

General Flow is:

if shouldExportMetrics:
     go checkIfOnline()
skaffold.Main()
if shouldExportMetrics:
     return
InitCloudMonitoring()
if noCredentials:
     return
readPreviousMetricsFromFile()
if !online:
     return writeToFile()
startExporter() # p.Start()
createMetrics()
uploadMetrics() # p.Stop() triggers uploading metrics

Follow-up Work

  • Create more extensive test for exporting of metrics.
  • Determine which flags to upload in the error metrics and command metrics
  • Address the ~5s latency of uploading metrics that locks the terminal by running it in a go routine and starting it concurrently during the end of normal skaffold execution.
  • Set meter.Command prior to the creation of the runner in order to refine collecting error metrics when they happen before the runner is created.
  • Fix bug where metrics written to a file are not unmarshalled correctly.

@IsaacPD IsaacPD requested a review from a team as a code owner December 16, 2020 02:24
@IsaacPD IsaacPD requested a review from MarlonGamez December 16, 2020 02:24
@google-cla google-cla bot added the cla: yes label Dec 16, 2020
@IsaacPD IsaacPD force-pushed the upload-metrics branch 2 times, most recently from 1dbb410 to 3c868c9 Compare December 16, 2020 14:59
@IsaacPD IsaacPD added kokoro:run runs the kokoro jobs on a PR and removed kokoro:run runs the kokoro jobs on a PR labels Dec 16, 2020
@IsaacPD IsaacPD force-pushed the upload-metrics branch 2 times, most recently from 6e72827 to 6cf2c24 Compare December 16, 2020 16:10
@codecov
Copy link

codecov bot commented Dec 16, 2020

Codecov Report

Merging #5157 (fb3c3ba) into master (6dbe2d2) will decrease coverage by 0.49%.
The diff coverage is 14.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5157      +/-   ##
==========================================
- Coverage   72.35%   71.85%   -0.50%     
==========================================
  Files         382      384       +2     
  Lines       13589    13729     +140     
==========================================
+ Hits         9832     9865      +33     
- Misses       3038     3138     +100     
- Partials      719      726       +7     
Impacted Files Coverage Δ
pkg/skaffold/instrumentation/meter.go 23.02% <14.03%> (-41.50%) ⬇️
cmd/skaffold/app/tips/tips.go 54.54% <0.00%> (-12.13%) ⬇️
cmd/skaffold/app/cmd/util.go 65.71% <0.00%> (-0.96%) ⬇️
pkg/skaffold/runner/runner.go 0.00% <0.00%> (ø)
pkg/skaffold/initializer/build/util.go 100.00% <0.00%> (ø)
cmd/skaffold/app/cmd/test.go 53.84% <0.00%> (ø)
pkg/skaffold/initializer/transparent.go 80.00% <0.00%> (ø)
cmd/skaffold/app/cmd/cmd.go 66.15% <0.00%> (+0.26%) ⬆️
pkg/skaffold/runner/build_deploy.go 69.89% <0.00%> (+1.00%) ⬆️

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 6dbe2d2...fb3c3ba. Read the comment docs.

@MarlonGamez
Copy link
Contributor

Took a first pass look at this and it seems good, I think I'd like another pair of eyes on this tho as there's a lot here. I'll take a second pass to make sure I'm not missing anything

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.

have one super tiny nit but from what I can gather from the otel docs this seems good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/metrics Metrics tracking work for skaffold cla: yes size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants