Implemented the sender worker for metrics #676
Conversation
| @@ -39,3 +39,15 @@ func (c *Client) CleanupOldMetrics() error { | |||
| } | |||
| return results.OneError() | |||
| } | |||
|
|
|||
| func (c *Client) SendMetrics() error { | |||
There was a problem hiding this comment.
Need comments on exported functions
mattyw
force-pushed
the
add-metric-sender-worker
branch
from
a38ea1e
to
c50c2e0
Compare
mattyw
changed the title
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
err = iter.Close()
if err != nil {
return err
}
mattyw
force-pushed
the
add-metric-sender-worker
branch
from
bb57fa6
to
730c238
Compare
| if iter.Next(&doc) { | ||
| batch[i] = &MetricBatch{st: st, doc: doc} | ||
| } else { | ||
| if iter.Err() != nil { |
There was a problem hiding this comment.
As a guard against inconsistency, how about:
if err := iter.Err(); err != nil {
return err
}
|
LGTM with one minor change commented above. I'd be ok landing this and following it up with more implementation in subsequent PRs. |
| } | ||
| } | ||
| for _, m := range batch { | ||
| err := m.SetSent() |
There was a problem hiding this comment.
It'd be nicer to set them all to sent in one go here, I think?
|
Mostly looking good, but needs a bit more work |
| @@ -78,7 +80,7 @@ func (s *metricsManagerSuite) TestNewMetricsManagerAPIRefusesNonClient(c *gc.C) | |||
| c.Assert(err, gc.ErrorMatches, "permission denied") | |||
| } | |||
|
|
|||
| func (s *metricsManagerSuite) TestCleanupArgsIndependant(c *gc.C) { | |||
| func (s *metricsManagerSuite) TestCleanupArgsIndependent(c *gc.C) { | |||
| args := params.Entities{Entities: []params.Entity{ | |||
| params.Entity{"invalid"}, | |||
| params.Entity{s.State.EnvironTag().String()}, | |||
There was a problem hiding this comment.
{"invalid"},
{s.State.EnvironTag().String()},Running gofmt -s -w will automagically change it.
There was a problem hiding this comment.
@waigani Thank you very much. Didn't know about -s
mattyw
force-pushed
the
add-metric-sender-worker
branch
from
d1667a5
to
87173b3
Compare
mattyw
force-pushed
the
add-metric-sender-worker
branch
from
87173b3
to
82ba5bd
Compare
added more mocking in tests support bulk calls in the sendmetrics api call
mattyw
force-pushed
the
add-metric-sender-worker
branch
from
82ba5bd
to
c9b281b
Compare
| @@ -23,6 +23,10 @@ type metricsManagerSuite struct { | |||
|
|
|||
| var _ = gc.Suite(&metricsManagerSuite{}) | |||
|
|
|||
| func TestAll(t *stdtesting.T) { | |||
There was a problem hiding this comment.
would you put this in a package_test.go please? it's the easiest way to make sure there's only one of them
|
essentially LGTM with a bunch of trivials |
|
|
||
| // SendMetrics will send any unsent metrics | ||
| // over the MetricSender interface in batches | ||
| // of no more than 1k. |
There was a problem hiding this comment.
I assume the size of 1k depends on batchSize
|
LGTM with a few minor nitpicks |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
Implemented the sender worker for metrics Implemented a sender worker that will send metrics to a given service
Implemented a sender worker that will send metrics to a given service