[bugfix] temporary fix around grpclogger #5272
Conversation
Calling SetLoggerV2 isn't thread safe and should only be called before any grpc functions. See more details: https://pkg.go.dev/github.com/grpc-ecosystem/go-grpc-middleware@v1.3.0/logging/settable#pkg-overview
Codecov Report
@@ Coverage Diff @@
## main #5272 +/- ##
==========================================
- Coverage 90.55% 90.53% -0.02%
==========================================
Files 188 189 +1
Lines 11101 11116 +15
==========================================
+ Hits 10052 10064 +12
- Misses 827 829 +2
- Partials 222 223 +1
Continue to review full report at Codecov.
|
This allows the InProcessCollector to prevent entering a race condition.
| @@ -64,6 +64,9 @@ type CollectorSettings struct { | |||
| // LoggingOptions provides a way to change behavior of zap logging. | |||
| LoggingOptions []zap.Option | |||
|
|
|||
| // SkipSettingGRPCLogger avoids setting the grpc logger | |||
| SkipSettingGRPCLogger bool | |||
There was a problem hiding this comment.
I might still be in need of a coffee, but how does this work? This is a new value, and I see this value only being read, never written, so, it will always be false. The conditional on service/collector.go would then always yield true, which will call the same code as before this PR.
There was a problem hiding this comment.
I think it will be set in correctness tests, which are in the contrib repo, not in this repo.
BTW, I am not sure why we moved the all of the testbed from core to contrib. I consider the testbed to be a core part of the project. We run the testbed for otlp receiver/exporter in this repo, now we don't anymore and any problem discovery is delayed until the contrib consumes the new core.
There was a problem hiding this comment.
I do agree that maybe the framework of testbed can live here, the reason we moved it was because we don't want to give API guarantees to the framework, and core is closer and closer to offer also API stability.
There was a problem hiding this comment.
the reason we moved it was because we don't want to give API guarantees to the framework
That's a good point. Maybe we can keep testbed go module on 0.x forever to indicate it is unstable and also have explicit warning that no-one should use this API except contrib (yes, I know people don't pay attention to warnings :-) ).
|
This fixed #4971; It's a good temporary fix but I don't love this because I feel like it's yet another individual flag for customizing telemetry, and I would like to instead have something more general like the telemetry providers I proposed on #4970. I haven't had time recently to push for that but I intend to continue the work there in the next few weeks, and would appreciate comments/thoughts/feedback from @codeboten and others who looked into this problem on that issue, to see if it is a good solution. |
|
I'll take a look and provide feedback after today's release @mx-psi |
Calling SetLoggerV2 isn't thread safe and should only be called
before any grpc functions. See more details: https://pkg.go.dev/github.com/grpc-ecosystem/go-grpc-middleware@v1.3.0/logging/settable#pkg-overviewThis hacky workaround checks if the collector's logger has been set, if so, don't bother updating the grpc logger. Looking for alternative implementations here.