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

Log with Logrus; allow for logger injection #528

Closed

Conversation

trevrosen
Copy link
Collaborator

@trevrosen trevrosen commented May 12, 2018

Fixes #508

What

  • Adds Logrus to the build
  • Replaces usage of log package from std lib with Logrus
  • Adds RegisterLogger functions to api and gobot packages

Why

Questions

  • Does this solve the request in Override the default log with logrus? #508? (cc @ferventgeek)
  • Should we leave the examples directory alone on the theory that it's best to show only Gobot packages and stdlib there? I opted to change b/c of the injection behavior's availability, but could see an argument for keeping it pure stdlib.

@trevrosen
Copy link
Collaborator Author

@ferventgeek please have a look here when you get a chance and let me know if this is what you have in mind or if you'd like to see further extension. Also: as we appear to have the same employer, feel free to get in touch IRL any time re Gobot. 😺

@trevrosen
Copy link
Collaborator Author

@deadprogram the thought occurs that we could go a step further and formalize a method of log support at either the driver or platform level. I've got a couple ideas around what that could look like.

@trevrosen
Copy link
Collaborator Author

trevrosen commented Jun 10, 2018

@deadprogram any thoughts on whether we should continue w/ merging here? I'll update to fix the deps conflict if so. I'd also like to squash merge this one if possible due to my own vain desire to keep some of my derping around from the log and reduce noise 😃

@trevrosen trevrosen closed this Jun 10, 2018
@trevrosen trevrosen reopened this Jun 10, 2018
@trevrosen trevrosen force-pushed the feature/508/logrus-for-logging branch from 077b774 to 9ee6b8b Compare June 10, 2018 16:56
@codecov
Copy link

codecov bot commented Jun 10, 2018

Codecov Report

Merging #528 into dev will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #528      +/-   ##
==========================================
- Coverage   51.49%   51.48%   -0.02%     
==========================================
  Files         162      162              
  Lines       15536    15540       +4     
==========================================
  Hits         8001     8001              
- Misses       7133     7137       +4     
  Partials      402      402
Impacted Files Coverage Δ
drivers/i2c/mcp23017_driver.go 91.25% <ø> (ø) ⬆️
device.go 100% <ø> (ø) ⬆️
drivers/i2c/adafruit_driver.go 74.46% <ø> (ø) ⬆️
platforms/ble/ble_client_adaptor.go 12.19% <ø> (ø) ⬆️
platforms/audio/audio_adaptor.go 80% <ø> (ø) ⬆️
platforms/keyboard/keyboard_driver.go 38.23% <ø> (ø) ⬆️
robot.go 100% <ø> (ø) ⬆️
connection.go 92.3% <0%> (-7.7%) ⬇️
api/api.go 94.8% <0%> (-0.83%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f859ff...9ee6b8b. Read the comment docs.

@trevrosen
Copy link
Collaborator Author

Closing. May re-open later.

@trevrosen trevrosen closed this Dec 19, 2018
@ferventgeek
Copy link

This is awesome, thanks for the update. Logging refactoring is always tricky- this will really help with IoT projects and random robot images with no serial connector. I'll have time to test in early January.

@trevrosen
Copy link
Collaborator Author

The PR is closed now after being open for comment for 7 months. I’m not satisfied with the change as it stood anyway. May revisit at some point in the future.

@ferventgeek
Copy link

Ahh, well it happens. Would still like integrated exportable logs, but then again who doesn't. ;-) I'm all over the Arduino/ESP32 gizmo over the holiday anyway. ;-)

@NikolaeVarius
Copy link
Contributor

So, what were you not satisfied with? Is there some sort of design goal we want to work torward with logging?

I'm specifically interested in how we might want to handle telemetry data. I think that should be considered a seperate thread alongside logging.

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.

3 participants