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.

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

OpenTelemetry Change Clarification #571

Open
joe94 opened this issue Mar 22, 2021 · 3 comments
Open

OpenTelemetry Change Clarification #571

joe94 opened this issue Mar 22, 2021 · 3 comments

Comments

@joe94
Copy link
Member

joe94 commented Mar 22, 2021

This relates to #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
Copy link
Contributor

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
Copy link
Member Author

joe94 commented Mar 30, 2021

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
Copy link
Member Author

joe94 commented Mar 30, 2021

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 xmidt-org/tr1d1um#202? If you need more information, LMK and I'm happy to provide some sample code for Talaria as well 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants