Skip to content

RSDK-5985 - per-resource logging #769

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

Merged

Conversation

stuqdog
Copy link
Member

@stuqdog stuqdog commented Oct 16, 2024

Adds support for per-resource logging, and propagates log-level changes for modular resources to the viam-server.

    robot = await connect()
    arm_with_debug_logging = Arm.from_robot(robot, 'myarm') # configured to debug log level
    arm_with_error_logging = Arm.from_robot(robot, 'myarm2') # configured to error log level
    arm_with_default_logging = Arm.from_robot(robot, 'myarm-copy') # no log level set in config

    moving = await arm_with_debug_logging.is_moving()
    moving2 = await arm_with_error_logging.is_moving()
    moving3 = await arm_with_default_logging.is_moving()

Output (ignore the motor logs and sorry for the noise!):
Screenshot 2024-10-16 at 13 44 53

Two notes/limitations:

  1. Currently debug logs don't propagate correctly to app from the viam-server, so while debug logs will show up on your terminal where you're running the server, you won't see them in app.viam.com. team netcode is aware of this and looking into it.
  2. Currently when constructing resources, we don't have access to the resource config or log level for setting. As soon as a resource is constructed we set the level, but this means that if, e.g., you had debug logs in a resource constructor, those logs would not propagate correctly unless the server was also set to debug logging. It would be possible to pass along log levels to constructors by updating their constructors but this seemed far more invasive of a PR and had the potential to break existing workflows; happy to discuss further if anyone disagrees or has questions or concerns.

@stuqdog stuqdog requested a review from a team as a code owner October 16, 2024 18:01
@stuqdog stuqdog requested review from gloriacai01, lia-viam and njooma and removed request for gloriacai01 October 16, 2024 18:01
@stuqdog
Copy link
Member Author

stuqdog commented Oct 16, 2024

@dgottlieb @benjirewis fyi

@stuqdog
Copy link
Member Author

stuqdog commented Oct 17, 2024

note to reviewers: definitely still able to be reviewed, but this won't be merged until some upstream changes have been made and testing has been done to confirm they work from the python sdk.

@stuqdog stuqdog changed the title per-resource logging RSDK-5985 - per-resource logging Oct 28, 2024
@stuqdog stuqdog merged commit c7b3d88 into viamrobotics:main Oct 30, 2024
12 checks passed
@stuqdog stuqdog deleted the propagate-log-level-changes-to-modules branch October 30, 2024 16:27
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

Successfully merging this pull request may close these issues.

2 participants