Change samples to use log instead of glog. #581
Conversation
google-prow-robot
added
the
size/M
| fmt.Fprintf(w, "%s not found for ticker : %s \n",resource, stockId) | ||
| if err != nil { | ||
| fmt.Fprintf(w, "%s not found for ticker : %s \n", resource, stockId) | ||
| return |
There was a problem hiding this comment.
Thanks @mdemirhan! I noticed sample/helloworld/updated_configuration.yaml still has the glog startup args in it. Also, would it be worth adding a "Best Practices" section to samples/README.md explaining that we prefer log?
| @@ -73,7 +72,8 @@ func init() { | |||
|
|
|||
| func main() { | |||
| flag.Parse() | |||
| glog.Info("Telemetry sample started.") | |||
| log.SetPrefix("TelemetrySample: ") | |||
There was a problem hiding this comment.
Why set prefix here but not in the other samples?
There was a problem hiding this comment.
No reason really. The prefix seems like a nice touch but doesn't really make or break anything. That is why I didn't do it in other samples.
There was a problem hiding this comment.
@grantr For the updated_configuration.yaml, this is a great catch. I will fix that.
As for why we prefer log vs glog: We didn't pick log because it is better than glog. I think the goal is to provide samples with minimal dependencies and let people pick the logging library they like the most.
There was a problem hiding this comment.
👍 on prefix. I agree the fewer statements the better in sample code, so I'm fine with leaving it out. IIUC the telemetry sample logs from multiple processes so having prefixes might be useful for aggregated logs.
I could have been more clear on the best practices thing, sorry about that. You said it well with, "our goal is to provide samples with minimal dependencies." That's the best practice, with one example being choosing log over glog, not because log is better, but because it's one less dependency.
I may be misunderstanding your intent though. If you only want to prefer log for current samples, and allow future samples to ignore this preference, then maybe it shouldn't be mentioned at all.
There was a problem hiding this comment.
@grantr I misunderstood you before and I thought the best practices were aimed at the users of Elafros, but not sample authors. It definitely makes sense to add such a section for sample authors. Thanks, I will add that momentarily and send an update.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: grantr, mdemirhan Assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@mdemirhan you shouldn't need to set up the DNS to test the webhook. Just creating ela resources will hit the webhook. Edit: Misread, you were talking about the gitwebhook, ack :) |
Fixes Issue #347
Proposed Changes
Chris Broadfoot proposed to use the standard log library rather than glog. Reasons being:
Instead of glog, we will use the standard go log library. If users want to swap it out for something custom, they can go for it.
Testing
I manually tested all the samples to ensure that the logs are showing up and apps are not crashing.
@vaikas-google I couldn't fully test gitwebhook sample. I was able to start it and see the startup log message, but I didn't go through the full DNS setup and see it working end-to-end. Let me know if you have any concerns.