Page MenuHomePhabricator

[services] Tunnelbroker - Use glog instead of `std::cout`
ClosedPublic

Authored by max on May 9 2022, 7:15 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 15, 6:10 AM
Unknown Object (File)
Thu, Nov 14, 9:08 AM
Unknown Object (File)
Thu, Nov 14, 9:08 AM
Unknown Object (File)
Sat, Nov 2, 3:39 PM
Unknown Object (File)
Oct 20 2024, 6:39 PM
Unknown Object (File)
Oct 7 2024, 10:14 PM
Unknown Object (File)
Oct 7 2024, 10:14 PM
Unknown Object (File)
Oct 7 2024, 10:14 PM

Details

Summary

Following our convention to use glog, convert the std:cout messages to the LOG format with different log levels:

  • INFO: For debug purposes,
  • ERROR: For errors.

Related linear task: ENG-801

Test Plan

Start the service in INFO and ERROR-only modes.

Diff Detail

Repository
rCOMM Comm
Branch
tunnelbroker-glog
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.May 9 2022, 8:18 AM
Harbormaster failed remote builds in B8929: Diff 12438!
tomek requested changes to this revision.May 10 2022, 12:25 AM

Following our convention to use glog

Could you remind me where this convention exists? I've searched the codebase and haven't found any usage of it. Also, do we need to install it somehow?

This revision now requires changes to proceed.May 10 2022, 12:25 AM
In D3969#111452, @palys-swm wrote:

Following our convention to use glog

Could you remind me where this convention exists?

The related task is ENG-801.
Before this task, we had a discussion in the Comm Dev Team chat and voted for glog because we already have it as a dependency from Folly.

Also, do we need to install it somehow?

No, because we already installed it as a dependency for Folly.

I've searched the codebase and haven't found any usage of it?

This is the first glog usage in our code (our Folly dependency uses it internally).

tomek added 1 blocking reviewer(s): karol.

This is the first glog usage in our code (our Folly dependency uses it internally).

Are we sure that we don't need to explicitly add glog as our dependency? @karol-bisztyga what's your opinion?

This is a good point, I think since we explicitly use it ourselves and it's no longer just a folly dependency for us, then I think we should extract its installation into a separate script. I'm going to accept this as it looks ok to me but we should follow up about this.

This revision is now accepted and ready to land.May 13 2022, 6:01 AM

Given @karol-bisztyga's comment, it is critical that we create a Linear task for the follow-up before landing this diff

Given @karol-bisztyga's comment, it is critical that we create a Linear task for the follow-up before landing this diff

Sure! ENG-1144 was created for this one.