-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Set grpc logger to collector #4501
Set grpc logger to collector #4501
Conversation
- used grpc_settable instead of grpc_zap ReplaceV2 directly as it supports multiple sets in thread safe fashoin as opposed to grpc_zap.ReplaceV2
Codecov Report
@@ Coverage Diff @@
## main #4501 +/- ##
==========================================
+ Coverage 90.67% 90.70% +0.02%
==========================================
Files 179 179
Lines 10414 10445 +31
==========================================
+ Hits 9443 9474 +31
Misses 755 755
Partials 216 216
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like the right direction to me, we just need to settle on the opentracing dependency that is brought by go-grpc-middleware.
@bogdandrutu @tigrannajaryan can we please get either of your blessing on this PR and let me know if anything else is needed. Please and Thank you. @jpkrohling Also I do see I can update our internal teams using collector with a workaround solution involving environment variables accordingly. Thanks you cc: @svrnm |
I don't think that label has any meaning by now, don't worry about it. @bogdandrutu, @tigrannajaryan, we need a maintainer to merge this, possibly doing a final review (despite having three approvals). |
Before merging this I would like to understand how chatty is gRPC in WARN level. For example if there is no connection to the destination does it periodically print warnings? Can this flood the logs? |
Thank you @tigrannajaryan - In case of default collector setting - which sets gRPC logging to WARN - We will have 1 message per connection failure per retry for exporter. 2021-11-30T16:47:38.384-0800 warn zapgrpc/zapgrpc.go:191 [core] grpc: addrConn.createTransport failed to connect to {otelcol:4317 otelcol:4317 <nil> <nil> 0 <nil>}. Err: connection error: desc = "transport: Error while dialing dial tcp: lookup otelcol: no such host" I have not tested for all other failures - but quickly glancing the gRPC code. The error specific verbose logs only happen to appear in INFO level. On the other hand - once we set debug logging on collector - we will have more chatty messages about gRPC transport 2021-11-30T17:35:09.442-0800 info zapgrpc/zapgrpc.go:174 [core] original dial target is: "otelcol:4317"
2021-11-30T17:35:09.443-0800 info zapgrpc/zapgrpc.go:174 [core] parsed dial target is: {Scheme:otelcol Authority: Endpoint:4317 URL:{Scheme:otelcol Opaque:4317 User: Host: Path: RawPath: ForceQuery:false RawQuery: Fragment: RawFragment:}}
2021-11-30T17:35:09.443-0800 info zapgrpc/zapgrpc.go:174 [core] fallback to scheme "passthrough"
2021-11-30T17:35:09.443-0800 info zapgrpc/zapgrpc.go:174 [core] parsed dial target is: {Scheme:passthrough Authority: Endpoint:otelcol:4317 URL:{Scheme:passthrough Opaque: User: Host: Path:/otelcol:4317 RawPath: ForceQuery:false RawQuery: Fragment: RawFragment:}}
2021-11-30T17:35:09.443-0800 info zapgrpc/zapgrpc.go:174 [core] Channel authority set to "otelcol:4317"
2021-11-30T17:35:09.443-0800 info zapgrpc/zapgrpc.go:174 [core] ccResolverWrapper: sending update to cc: {[{otelcol:4317 <nil> <nil> 0 <nil>}] <nil> <nil>}
2021-11-30T17:35:09.443-0800 info zapgrpc/zapgrpc.go:174 [core] ClientConn switching balancer to "pick_first"
2021-11-30T17:35:09.443-0800 info zapgrpc/zapgrpc.go:174 [core] Channel switches to new LB policy "pick_first" plus in case of failure (for no host error) we will have these set of error specific messages per retry cycle of the exporter.
The default behavior of gRPC framework itself is to set logging level to ERR and write logs to `ioutil.DIscard. |
This sounds ok to me. Let's merge this but be prepared to adjust the default behavior if it turns out to be more noisy than we would want. |
Thanks @tigrannajaryan . Please let me know if you need anything else from my end. |
Description:
Adding a feature - This feature sets the grpc logger to one derived from collector logger configuration and enables users to control grpc logs.
in non development made
DEBUG in development mode
produces
Link to tracking Issue: #2237
Testing: Added unit tests and manual testing