-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: expose BatchCounter for MetricBatcher #28
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,18 +1,18 @@ | ||||||
// Package metrics provides a simple API for sending value and counter metrics | ||||||
// through the dropsonde system. | ||||||
// | ||||||
// Use | ||||||
// # Use | ||||||
// | ||||||
// See the documentation for package dropsonde for configuration details. | ||||||
// | ||||||
// Importing package dropsonde and initializing will initial this package. | ||||||
// To send metrics use | ||||||
// | ||||||
// metrics.SendValue(name, value, unit) | ||||||
// metrics.SendValue(name, value, unit) | ||||||
// | ||||||
// for sending known quantities, and | ||||||
// | ||||||
// metrics.IncrementCounter(name) | ||||||
// metrics.IncrementCounter(name) | ||||||
// | ||||||
// to increment a counter. (Note that the value of the counter is maintained by | ||||||
// the receiver of the counter events, not the application that includes this | ||||||
|
@@ -21,6 +21,7 @@ package metrics | |||||
|
||||||
import ( | ||||||
"github.com/cloudfoundry/dropsonde/metric_sender" | ||||||
"github.com/cloudfoundry/dropsonde/metricbatcher" | ||||||
"github.com/cloudfoundry/sonde-go/events" | ||||||
) | ||||||
|
||||||
|
@@ -52,6 +53,7 @@ type MetricBatcher interface { | |||||
BatchIncrementCounter(name string) | ||||||
BatchAddCounter(name string, delta uint64) | ||||||
Close() | ||||||
BatchCounter(name string) metricbatcher.BatchCounterChainer | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the camel case is lost here, it does not correspond to the other code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as it is imported library name, I will not change it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name is not longer and more readable in camel case, that' s why I would prefer that. It was only a suggestion, of course you are free to keep it. |
||||||
} | ||||||
|
||||||
// Initialize prepares the metrics package for use with the automatic Emitter. | ||||||
|
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.
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 agree with @b1tamara, the imported package should not be aliased in camel case notation.
@Dariquest
Good package names are short and clear. They are lower case, with no under_scores or mixedCaps.
. Check the whole blogpost on package names for more information ;)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.
Thanks for the blogpost @chombium. The name is not longer and more readable in camel case. Only imho. My suggestion might be more appropriate for other programming languages then Go, which I am used to :)
The package names should be at least unified, what I can find here is unfortunately:
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.
@Dariquest I know what you mean and I would completely agree if it wasn't Go. Go has some different conventions. On the other hand, we have the same import used like here in many places in other CF components.