-
Notifications
You must be signed in to change notification settings - Fork 327
Remove error from stats.{Float,Int}64Measure signatures #641
Remove error from stats.{Float,Int}64Measure signatures #641
Conversation
5059900
to
6af8039
Compare
6af8039
to
f5b59f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I've added a minor suggestion. Thank you @Ramonza!
@@ -49,13 +53,6 @@ func main() { | |||
} | |||
view.RegisterExporter(exporter) | |||
|
|||
// Create measures. The program will record measures for the size of | |||
// processed videos and the nubmer of videos marked as spam. | |||
videoSize, err := stats.Int64("my.org/measure/video_size", "size of processed videos", "MBy") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about let's bring that back in here instead of creating it as a global variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? we encourage users to declare these as globals. major justification for this change was to make declaring as globals easier so thought it appropriate to demonstrate this here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, then sure, let's use it in not only main but say in another helper, otherwise as it is, doesn't look justified to be a global.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will extract the record into a separate function. I think having multiple files for an example would be too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In deed. Not multiple files though, just multiple functions.
internal/readme/stats.go
Outdated
@@ -26,14 +26,11 @@ import ( | |||
// README.md is generated with the examples here by using embedmd. | |||
// For more details, see https://github.com/rakyll/embedmd. | |||
|
|||
var videoSize = stats.Int64("my.org/video_size", "processed video size", "MB") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto about declaring videoSize where it was originally.
f5b59f9
to
8440347
Compare
8440347
to
7b734ca
Compare
stats.Int64Measure no longer returns an error as of: census-instrumentation/opencensus-go#641 Change-Id: I14340f8c0d975c84143dae745be60cfd0c5a4003 Reviewed-on: https://code-review.googlesource.com/25970 Reviewed-by: Jonathan Amsterdam <[email protected]>
Fixes: #544