Skip to content

otelgrpc: StreamClientInterceptor ends spans synchronously#4537

Merged
MadVikingGod merged 10 commits intoopen-telemetry:mainfrom
pellared:StreamClientInterceptor-sync
Nov 14, 2023
Merged

otelgrpc: StreamClientInterceptor ends spans synchronously#4537
MadVikingGod merged 10 commits intoopen-telemetry:mainfrom
pellared:StreamClientInterceptor-sync

Conversation

@pellared
Copy link
Copy Markdown
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.

@codecov
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 pellared force-pushed the StreamClientInterceptor-sync branch from 46b73b1 to 5763eb3 Compare November 10, 2023 11:55
@pellared
Copy link
Copy Markdown
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
@pellared pellared deleted the StreamClientInterceptor-sync branch November 14, 2023 15:50
@pellared pellared added this to the untracked milestone Nov 8, 2024
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