-
Notifications
You must be signed in to change notification settings - Fork 329
doc: add SetReportingPeriod recommendations #910
doc: add SetReportingPeriod recommendations #910
Conversation
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, thank you @jadekler! There is a minor nit, PTAL.
stats/view/worker.go
Outdated
@@ -113,11 +113,15 @@ func record(tags *tag.Map, ms interface{}) { | |||
ms: ms.([]stats.Measurement), | |||
} | |||
defaultWorker.c <- req | |||
} | |||
}; |
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.
Please remove this, this ain't Java 😂
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.
Done
// the program. If duration is less than or equal to zero, it enables the | ||
// default behavior. | ||
// | ||
// Note: each exporter makes different promises about what the lowest supported |
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.
Nice, thank you! It would be nice to also mail a change to the actual stackdriver exporter repo providing an advisory for this time duration.
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.
@songy23 I see you initially mailed census-ecosystem/opencensus-go-exporter-stackdriver#37 but closed it. Will you perhaps be making an update in light of this new PR?
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.
Yes, the Stackdriver example is already updated in census-ecosystem/opencensus-go-exporter-stackdriver#38.
PTAL |
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
No description provided.