fix: grpc reconnection

[kstasik]

I have successfully reproduced this issue on 0.15.0 but i think it also applies to recent version. After disconnection it tries to reconnect only once. With this fix it should try reconnect periodically until successful connection.

Please let me know what do you think. Maybe there is some other better way to fix it.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Kacper Stasik (d7dbd5e)

[codecov]

Codecov Report

Merging #1521 (0597d42) into main (3bce9c9) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@          Coverage Diff          @@
##            main   #1521   +/-   ##
=====================================
  Coverage   78.0%   78.0%           
=====================================
  Files        127     127           
  Lines       6600    6600           
=====================================
+ Hits        5153    5154    +1     
+ Misses      1203    1202    -1     
  Partials     244     244           

Impacted Files	Coverage Δ
exporters/otlp/otlpgrpc/connection.go	86.9% <100.0%> (+0.9%)	⬆️

[MrAlias]

Is there an issue tracking this? If not, can we start there? It would be nice to better understand the problem this is attempting to solve.

[kstasik]

I hope it helps a little bit:

#1524

[jmacd]

It would be great if we had a test that would prevent regressions, since this bug has been a major issue of this nature. I am not very familiar with the code, but this certainly looks correct.

[XSAM]

I can't reproduce this bug, and the test TestNewExporter_collectorConnectionDiesThenReconnects works fine.

[XSAM]

Suggested change

	- Fixed otlpgrpc reconnection issue.
	- Fixed otlpgrpc reconnection issue. (#1521)

[kstasik]

thank you - i didn't notice integration tests. I'm not sure about my solution but I think this test doesn't cover this line:

https://github.com/open-telemetry/opentelemetry-go/blob/main/exporters/otlp/otlpgrpc/connection.go#L158

Which i think causes goroutine to hang. I will try to write new test or extend this.

[kstasik]

I think changing disconnectedCh channel to buffered may be better option but I will change this PR once i have time to write integration tests.

[punya]

This may also fix the test flake I ran into elsewhere (TestNewExporter_collectorConnectionDiesThenReconnects).

[punya]

I can't reproduce this bug, and the test TestNewExporter_collectorConnectionDiesThenReconnects works fine.

Actually, I saw it flake several times in CI yesterday, and locally I see it fail about 5% of the time (https://gist.githubusercontent.com/punya/1db2a5fc7aede3baa06685997e145f3b/raw/bc52d8e2ba68772539a36c87f8ff9d1492e2ecf6/runs.txt).

[kstasik]

btw. in this case connected() function was very misleading for me (grpc exporter). It doesn't mean exporter is connected and thanks to this reconnection goroutine it almost always returns true.

First reconnection always successfully creates new "connection" which is really not established.
https://pkg.go.dev/google.golang.org/grpc#DialContext

I changed fix to buffered channel, please review and let me know what do you think?

[XSAM]

Looks good 👍