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.

Sign up for GitHub

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

Jump to bottom

RSDK-9150 - Client log interceptor should not use zap.Logger #375

Merged
merged 5 commits into from
Oct 28, 2024
Merged

RSDK-9150 - Client log interceptor should not use zap.Logger #375

merged 5 commits into from
Oct 28, 2024

Conversation

cheukt
Copy link
Member

@cheukt cheukt commented Oct 25, 2024 •
edited
Loading

no reason to convert the logger to a zap logger and then stick it in the context

before changes:

2024-10-25T13:29:18.659-0400    ERROR   rdk.resource_manager.rdk:remote:/robot2.networking.networking.webrtc     zap/options.go:212 finished client unary call
{"system": "grpc", "span.kind": "client", "grpc.service": "viam.robot.v1.RobotService",
"grpc.method": "ResourceNames", "error": "rpc error: code = Unknown desc = ggg", "grpc.code": "Unknown", "grpc.time_ms": 0.6}

2024-10-25T15:14:21.806-0400    DEBUG   rdk.resource_manager.rdk:remote:/robot2.networking.networking.webrtc   zap/options.go:212 finished client unary call     
 {"system": "grpc", "span.kind": "client", "grpc.service": "viam.robot.v1.RobotService", "grpc.method": "ResourceNames", "grpc.code": "OK", "grpc.time_ms": 0.538}

after changes:

2024-10-25T19:20:50.385Z        ERROR   rdk.resource_manager.rdk:remote:/robot2.networking.networking.webrtc   rpc/wrtc_client_channel.go:358 finished client unary call
{"error":"rpc error: code = Unknown desc = hello","grpc.code":"Unknown","grpc.time_ms":0.76,"span.kind":"client","system":"grpc","grpc.service":"viam.robot.v1.RobotService","grpc.method":"ResourceNames"}

2024-10-25T19:19:17.405Z        DEBUG   rdk.resource_manager.rdk:remote:/robot2.networking.networking.webrtc   rpc/wrtc_client_channel.go:354 finished client unary call
{"grpc.code":"OK","grpc.time_ms":0.345,"span.kind":"client","system":"grpc","grpc.service":"viam.robot.v1.RobotService","grpc.method":"ResourceRPCSubtypes"}

field order is different, since zap loggers put the With() fields in front whereas the RDK logger puts them in the back

@viambot viambot added the safe to test Mark as safe to test label Oct 25, 2024
cheukt
cheukt commented Oct 25, 2024
View reviewed changes
cheukt
cheukt commented Oct 25, 2024
View reviewed changes
rpc/wrtc_client_channel.go Outdated
Comment on lines 352 to 363
switch level {
case zap.DebugLevel:
logger.Debugw(msg, fields...)
case zap.InfoLevel:
logger.Infow(msg, fields...)
case zap.ErrorLevel:
logger.Errorw(msg, fields...)
case zap.WarnLevel:
fallthrough
default:
logger.Warnw(msg, fields...)
}
Copy link
Member Author

@cheukt cheukt Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is pretty ugly, but the Write() methods differ between zap loggers and rdk loggers so I could either write another conversion method like AddFieldsToLogger or do this and this is easier

cheukt
cheukt commented Oct 25, 2024
View reviewed changes
rpc/wrtc_client_channel.go
zap.String("grpc.service", service),
zap.String("grpc.method", method),
return []any{
"span.kind", "client",
Copy link
Member Author

@cheukt cheukt Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: rdk loggers' WithFields() do not play nicely with zapcore.Field at all. which probably means that zap loggers will not play nicely with AddFieldsToLogger

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for filing that ticket; sounds like zap loggers will almost certainly not play nicely with AddFieldsToLogger. But, from the sample "after changes" output you put in the PR description, the added fields do seem to be getting added. Is that right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, but the way they're getting passed in now is different - no longer a list of zap.Fields

@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Oct 25, 2024
@cheukt cheukt changed the title RSDK-9150 - Client log interceptor should use logger passed in RSDK-9150 - Client log interceptor should not use zap.Logger Oct 25, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Oct 25, 2024
dgottlieb
dgottlieb reviewed Oct 26, 2024
View reviewed changes
rpc/wrtc_client_channel.go Outdated
}
fields = append(fields, "grpc.code", code.String(), "grpc.time_ms", duration)

if logger.Level().Enabled(level) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary? I would expect if the level were not enabled, then the calls to logger.Debuw/logger.Infow/etc... would early return. Are we not observing that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya this is probably unnecessary, let me check. I was thinking too much about replicating the functionality of grpc_zap.DefaultMessageProducer, which also does a check before logging

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, definitely speak up if removing this is problematic.

If removing is not problematic, then we can cascade and delete the interface change that added Level and the dummy implementations for the testing cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated to remove

benjirewis
benjirewis reviewed Oct 28, 2024
View reviewed changes
rpc/wrtc_client_channel.go
zap.String("grpc.service", service),
zap.String("grpc.method", method),
return []any{
"span.kind", "client",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for filing that ticket; sounds like zap loggers will almost certainly not play nicely with AddFieldsToLogger. But, from the sample "after changes" output you put in the PR description, the added fields do seem to be getting added. Is that right?

@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Oct 28, 2024
benjirewis
benjirewis approved these changes Oct 28, 2024
View reviewed changes
Copy link
Member

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@cheukt cheukt merged commit 5600d4a into viamrobotics:main Oct 28, 2024
6 checks passed
@cheukt cheukt deleted the fix-client-logs branch October 28, 2024 17:21
@bashar-515 bashar-515 mentioned this pull request Jan 8, 2025
Merged
@bashar-515 bashar-515 mentioned this pull request Jan 16, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgottlieb dgottlieb dgottlieb approved these changes

@benjirewis benjirewis benjirewis approved these changes

Assignees
No one assigned
Labels
safe to test Mark as safe to test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cheukt @dgottlieb @benjirewis @viambot