feat: expose BatchCounter for MetricBatcher #28
Conversation
|
@geofffranks Could you maybe review it? |
| @@ -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.
| BatchCounter(name string) metricbatcher.BatchCounterChainer | |
| BatchCounter(name string) metricBatcher.BatchCounterChainer |
There was a problem hiding this comment.
the camel case is lost here, it does not correspond to the other code
There was a problem hiding this comment.
as it is imported library name, I will not change it.
There was a problem hiding this comment.
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.
| @@ -21,6 +21,7 @@ package metrics | |||
|
|
|||
| import ( | |||
| "github.com/cloudfoundry/dropsonde/metric_sender" | |||
| "github.com/cloudfoundry/dropsonde/metricbatcher" | |||
There was a problem hiding this comment.
| "github.com/cloudfoundry/dropsonde/metricbatcher" | |
| "metricBatcher github.com/cloudfoundry/dropsonde/metricbatcher" |
There was a problem hiding this comment.
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.
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:
import (
"github.com/cloudfoundry/dropsonde/metric_sender"
"github.com/cloudfoundry/dropsonde/metricbatcher"
There was a problem hiding this comment.
@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.
Exposing BatchCounter allows to set Tags for the metrics (similar to metric sender ValueCounter)
Related to cloudfoundry/routing-release#445