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

Bump OpenTelemetry dependencies to v1.7.0 #14104

Closed
wants to merge 2 commits into from

Conversation

lucacome
Copy link

Bumps OpenTelemetry dependencies to v1.7.0

@ahrtr
Copy link
Member

ahrtr commented Jun 10, 2022

This is the probably 4th attempt to bump the OpenTelemetry. Note that all previous attempts were failed. It could be due to the behavior change on the new gRPC version.

It would be helpful if you get it resolved.

@lucacome
Copy link
Author

@ahrtr I noticed unfortunately 😭 but I wanted to give it a try

@lucacome
Copy link
Author

I think one of the problems is

go.etcd.io/etcd/raft/v3/raftpb imports
	github.com/golang/protobuf/proto tested by
	github.com/golang/protobuf/proto.test imports
	github.com/google/go-cmp/cmp imports
	github.com/google/go-cmp/cmp/internal/value loaded from github.com/google/[email protected],
	but go 1.16 would select v0.5.6

To upgrade to the versions selected by go 1.16:
	go mod tidy -go=1.16 && go mod tidy -go=1.17
If reproducibility with go 1.16 is not needed:
	go mod tidy -compat=1.17
For other options, see:
	https://golang.org/doc/modules/pruning

@ptabor
Copy link
Contributor

ptabor commented Jun 14, 2022

I was looking into this. The real problem is grpc upgrade and it seems that new grpc version is more strict about format of the 'connection targets'. In particular our tests depend a lot on unix://localhost:port, while
according to grpc standard:
https://grpc.github.io/grpc/cpp/md_doc_naming.html
the supported names are: unix:path or unix://absolute_path (so unix:///tmp/foo).

Etcd also assumes ability to parse out the port from the url... so there is a lot of legacy to scribe here.

@serathius
Copy link
Member

In particular our tests depend a lot on unix://localhost:port, while according to grpc standard: https://grpc.github.io/grpc/cpp/md_doc_naming.html the supported names are: unix:path or unix://absolute_path (so unix:///tmp/foo).

This is very important think to note. Thanks @ptabor for pointing this out I think we should working towards grpc url standards to avoid similar issues in future.

lucacome added 2 commits June 15, 2022 14:36
Signed-off-by: Luca Comellini <[email protected]>
@codecov-commenter
Copy link

Codecov Report

Merging #14104 (4a1cfad) into main (a1fe0b8) will decrease coverage by 0.19%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #14104      +/-   ##
==========================================
- Coverage   75.43%   75.24%   -0.20%     
==========================================
  Files         454      454              
  Lines       36938    36916      -22     
==========================================
- Hits        27864    27777      -87     
- Misses       7344     7411      +67     
+ Partials     1730     1728       -2     
Flag Coverage Δ
all 75.24% <ø> (-0.20%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
server/embed/config_tracing.go 18.33% <ø> (ø)
client/v3/snapshot/v3_snapshot.go 0.00% <0.00%> (-54.35%) ⬇️
server/auth/simple_token.go 80.00% <0.00%> (-8.47%) ⬇️
server/etcdserver/api/v3lock/lock.go 61.53% <0.00%> (-8.47%) ⬇️
server/storage/schema/membership.go 51.82% <0.00%> (-8.03%) ⬇️
server/etcdserver/cindex/cindex.go 84.90% <0.00%> (-7.55%) ⬇️
server/etcdserver/api/v2stats/queue.go 20.83% <0.00%> (-7.38%) ⬇️
server/storage/schema/cindex.go 94.87% <0.00%> (-5.13%) ⬇️
raft/rafttest/node.go 95.00% <0.00%> (-5.00%) ⬇️
client/v3/concurrency/session.go 88.63% <0.00%> (-4.55%) ⬇️
... and 27 more

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 a1fe0b8...4a1cfad. Read the comment docs.

@ptabor
Copy link
Contributor

ptabor commented Jun 17, 2022

Closing as it's done in #14125

@ptabor ptabor closed this Jun 17, 2022
@lucacome
Copy link
Author

@ptabor it looks like only grpc was updated, not otel? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants