xmidt-org / webpa-common

The collection of small common packages for the webpa project.
Apache License 2.0
25 stars 25 forks source link

OpenTelemetry Change Clarification #571

Open joe94 opened 3 years ago

joe94 commented 3 years ago

This relates to https://github.com/xmidt-org/webpa-common/pull/569

Our best guess as to what this change is proposing is ensuring that the device connection manager uses a logger that's enriched with tracing information but we'd like to know so we know how to help in finding a solution.

If we simply want to log a device registration event with tracing information, we do not need to modify the manager's logger because it might not make sense to use the same device registration tracing info in the logger that the device uses. This is because the device registration trace should end once the registration has either failed or succeeded.

utsavbatra5 commented 3 years ago

Yes. I agree with your comments here. Instead, we can inject the logger from context in the newDevice(deviceOptions{}) configuration instead of manager logger so that each device has their own logger with relevant span ID. Let me know your thoughts on the same so that we can generate a new PR for the same.

joe94 commented 3 years ago

So if we want to log tracing information for requests made to devices, a better place to pass a LoggerFunc (similar to https://github.com/xmidt-org/argus/blob/main/chrysom/client.go#L169) might be the wrpHandler https://github.com/xmidt-org/talaria/blob/main/WRPHandler.go#L29

FYI: All WRP messages that flow to devices go through this handler.

joe94 commented 3 years ago

On a slight tangent, @utsavbatra5. When adding the server-side OpenTelemetry integration to talaria, would you be interested in trying out the approach described here https://github.com/xmidt-org/tr1d1um/issues/202? If you need more information, LMK and I'm happy to provide some sample code for Talaria as well 🚀