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

update otel libs from v0.13.0 -> v0.20.0 #5757

Merged

Conversation

aaron-prindle
Copy link
Contributor

@aaron-prindle aaron-prindle commented Apr 29, 2021

What is the problem being solved?
Part of #5756, adding opentelemetry trace information to skaffold commands. Updating out libs to the latest otel version adds additional useful functionality for tracing.

Why is this the best approach?
This approach uses go mod (updated via "go get ") and minor API changes to our otel API usage for the update.

What other approaches did you consider?
N/A

What side effects will this approach have?
There shouldn't be any side effects w/ this approach, the changes to otel's API were renaming/moving things (packages, functions, etc.). The only option removed was stdout Quantile aggregation (stdout.WithQuantiles) but I do not think this will have side effects. See open-telemetry/opentelemetry-go@49f699d#diff-2b283a7fb9f9b66e31a2b51a9ae9cad3599650a633f02fea9a956c4f6a714c6c

What future work remains to be done?
N/A

@aaron-prindle aaron-prindle requested a review from a team as a code owner April 29, 2021 18:33
@google-cla google-cla bot added the cla: yes label Apr 29, 2021
@aaron-prindle aaron-prindle force-pushed the update-otel branch 2 times, most recently from 3b83e5d to ab10f92 Compare April 29, 2021 19:17
@pull-request-size pull-request-size bot added size/L and removed size/M labels Apr 29, 2021
Comment on lines 30 to 32
"go.opentelemetry.io/otel"
"go.opentelemetry.io/otel/exporters/stdout"
sdktrace "go.opentelemetry.io/otel/sdk/trace"
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like these 3rd party libs need to be in the 3rd party import block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! This is fixed now using gci tool

@aaron-prindle aaron-prindle force-pushed the update-otel branch 2 times, most recently from f4455e9 to 25242e4 Compare April 29, 2021 20:20
@pull-request-size pull-request-size bot added size/M and removed size/L labels Apr 29, 2021
@pull-request-size pull-request-size bot added size/L and removed size/M labels Apr 29, 2021
@codecov
Copy link

codecov bot commented Apr 29, 2021

Codecov Report

Merging #5757 (e3d2d9d) into master (11a27cb) will decrease coverage by 0.20%.
The diff coverage is 93.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5757      +/-   ##
==========================================
- Coverage   70.87%   70.67%   -0.21%     
==========================================
  Files         421      423       +2     
  Lines       16091    16165      +74     
==========================================
+ Hits        11405    11425      +20     
- Misses       3850     3900      +50     
- Partials      836      840       +4     
Impacted Files Coverage Δ
pkg/skaffold/instrumentation/export.go 73.10% <93.10%> (ø)
pkg/skaffold/kubernetes/log.go 47.41% <0.00%> (ø)
pkg/skaffold/schema/versions.go 82.41% <0.00%> (ø)
pkg/skaffold/kubernetes/colorpicker.go 100.00% <0.00%> (ø)
pkg/skaffold/schema/v2beta15/config.go 20.58% <0.00%> (ø)
pkg/skaffold/schema/v2beta15/upgrade.go 100.00% <0.00%> (ø)
pkg/skaffold/tag/util.go 90.90% <0.00%> (+1.43%) ⬆️

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 11a27cb...e3d2d9d. Read the comment docs.

What is the problem being solved?
Part of GoogleContainerTools#5756, adding opentelemetry trace information to skaffold commands. Updating out libs to the latest otel version adds additional useful functionality for tracing.

Why is this the best approach?
This approach uses go mod (updated via "go get <pkg>") and minor API changes to our otel API usage for the update.

What other approaches did you consider?
N/A

What side effects will this approach have?
There shouldn't be any side effects w/ this approach, the changes to otel's API were renaming/moving things (packages, functions, etc.).  The only option removed was stdout Quantile aggregation (stdout.WithQuantiles) but I do not think this will have side effects.  See open-telemetry/opentelemetry-go@49f699d#diff-2b283a7fb9f9b66e31a2b51a9ae9cad3599650a633f02fea9a956c4f6a714c6c

What future work remains to be done?
N/A
@briandealwis
Copy link
Member

Can you verify with SKAFFOLD_EXPORT_TO_STDOUT whether the quantiles is equivalent? Or that it's ingested appropriately with Cloud Monitoring?

@aaron-prindle
Copy link
Contributor Author

aaron-prindle commented Apr 30, 2021

The Quantile piece is only used for the stdout exporter (only used locally via env var SKAFFOLD_EXPORT_TO_STDOUT) which is not used in data export/ingestion for Cloud Monitoring. The way quantiles were used w/ the stdout exporter was not relevant to our logging setup (they were looked at in test but the quantile information was used similarly to value information in the test, no real quantile usage). This will have no import on data export/ingestion, metrics will still work as expected. I have verified that this setup still works as expected pointing this at my own project aprindle-metrics-test and firing metrics there:

Screenshot 2021-04-30 3 35 39 PM

image

@briandealwis briandealwis added the kokoro:run runs the kokoro jobs on a PR label May 2, 2021
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label May 2, 2021
@aaron-prindle aaron-prindle merged commit 91c4480 into GoogleContainerTools:master May 2, 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.

4 participants