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

otelgrpc: StreamClientInterceptor ends spans synchronously #4537

Merged

Conversation

pellared
Copy link
Member

@pellared pellared commented Nov 7, 2023

Fixes #1352

Even though we are deprecating the interceptors, I think we should still have them as https://pkg.go.dev/google.golang.org/grpc/stats is experimental API.

Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Merging #4537 (36ccc6c) into main (5ba7d1e) will increase coverage by 0.0%.
The diff coverage is 47.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #4537   +/-   ##
=====================================
  Coverage   80.8%   80.8%           
=====================================
  Files        150     150           
  Lines      10371   10342   -29     
=====================================
- Hits        8387    8364   -23     
+ Misses      1840    1834    -6     
  Partials     144     144           
Files Coverage Δ
...ion/google.golang.org/grpc/otelgrpc/interceptor.go 88.5% <47.0%> (+0.6%) ⬆️

@dashpole

This comment was marked as resolved.

@dashpole
Copy link
Contributor

dashpole commented Nov 9, 2023

This makes sense to me, but do we know why it was asynchronous in the first place?

@pellared
Copy link
Member Author

pellared commented Nov 9, 2023

This makes sense to me, but do we know why it was asynchronous in the first place?

I have no idea 😢

EDIT: It looks like it was implemented like this "from the beginning": https://github.com/DataDog/dd-trace-go/blob/adc86d4779d9186e509ad0aa51f85bbcb0597241/contrib/google.golang.org/grpc/client.go

@MadVikingGod
Copy link
Contributor

MadVikingGod commented Nov 9, 2023

Can we do a benchmark comparison between the two? If there isn't a significant change, then I approve.

@pellared
Copy link
Member Author

pellared commented Nov 10, 2023

Benchmarks added: 5763eb3

new.txt:

goos: linux
goarch: amd64
pkg: go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/test
cpu: Intel(R) Core(TM) i9-10885H CPU @ 2.40GHz
BenchmarkStreamClientInterceptor-16    	    2325	    532989 ns/op	  346889 B/op	     273 allocs/op
BenchmarkStreamClientInterceptor-16    	    2226	    536866 ns/op	  347221 B/op	     273 allocs/op
BenchmarkStreamClientInterceptor-16    	    2283	    598328 ns/op	  347083 B/op	     273 allocs/op
BenchmarkStreamClientInterceptor-16    	    2118	    583204 ns/op	  347681 B/op	     273 allocs/op
BenchmarkStreamClientInterceptor-16    	    2178	    575682 ns/op	  346662 B/op	     273 allocs/op
BenchmarkStreamClientInterceptor-16    	    2090	    564414 ns/op	  347022 B/op	     273 allocs/op
BenchmarkStreamClientInterceptor-16    	    2166	    587932 ns/op	  347035 B/op	     273 allocs/op
BenchmarkStreamClientInterceptor-16    	    1878	    574177 ns/op	  346924 B/op	     273 allocs/op
BenchmarkStreamClientInterceptor-16    	    1798	    625722 ns/op	  348389 B/op	     273 allocs/op
BenchmarkStreamClientInterceptor-16    	    2048	    567798 ns/op	  347773 B/op	     273 allocs/op
PASS
ok  	go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/test	13.680s

old.txt (I cherry picked 5763eb3 to main)

goos: linux
goarch: amd64
pkg: go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/test
cpu: Intel(R) Core(TM) i9-10885H CPU @ 2.40GHz
BenchmarkStreamClientInterceptor-16    	    2379	    573916 ns/op	  348100 B/op	     278 allocs/op
BenchmarkStreamClientInterceptor-16    	    1999	    588659 ns/op	  347884 B/op	     279 allocs/op
BenchmarkStreamClientInterceptor-16    	    2029	    556518 ns/op	  348377 B/op	     278 allocs/op
BenchmarkStreamClientInterceptor-16    	    2179	    580318 ns/op	  348030 B/op	     279 allocs/op
BenchmarkStreamClientInterceptor-16    	    2038	    581189 ns/op	  347701 B/op	     278 allocs/op
BenchmarkStreamClientInterceptor-16    	    2049	    543641 ns/op	  348784 B/op	     278 allocs/op
BenchmarkStreamClientInterceptor-16    	    1768	    600244 ns/op	  349069 B/op	     279 allocs/op
BenchmarkStreamClientInterceptor-16    	    1975	    584428 ns/op	  348172 B/op	     278 allocs/op
BenchmarkStreamClientInterceptor-16    	    2094	    576019 ns/op	  348038 B/op	     278 allocs/op
BenchmarkStreamClientInterceptor-16    	    1906	    612972 ns/op	  348779 B/op	     279 allocs/op
PASS
ok  	go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/test	14.457s

benchstat (no significant change):

goos: linux
goarch: amd64
pkg: go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/test
cpu: Intel(R) Core(TM) i9-10885H CPU @ 2.40GHz
                           │   old.txt   │            new.txt            │
                           │   sec/op    │   sec/op     vs base          │
StreamClientInterceptor-16   580.8µ ± 4%   574.9µ ± 7%  ~ (p=0.529 n=10)

                           │   old.txt    │               new.txt               │
                           │     B/op     │     B/op      vs base               │
StreamClientInterceptor-16   340.0Ki ± 0%   338.9Ki ± 0%  -0.31% (p=0.001 n=10)

                           │  old.txt   │              new.txt              │
                           │ allocs/op  │ allocs/op   vs base               │
StreamClientInterceptor-16   278.0 ± 0%   273.0 ± 0%  -1.80% (p=0.000 n=10)

@MadVikingGod MadVikingGod merged commit 7469f61 into open-telemetry:main Nov 14, 2023
21 of 22 checks passed
@pellared pellared deleted the StreamClientInterceptor-sync branch November 14, 2023 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky test: TestInterceptors/StreamClientSpans
3 participants