[Do not merge]Consume latest tally #334

ghost · 2017-02-24T20:24:53Z

Tally introduced histograms to their interfaces and changed constructor for a scope, which requires us to implement it for NopReported and update some tests.

Will not merge, until migrate internal tools to the new tally version.

mention-bot · 2017-02-24T20:24:54Z

@alsamylkin, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sectioneight, @madhuravi and @glibsm to be potential reviewers.

coveralls · 2017-02-24T20:28:14Z

Coverage decreased (-0.4%) to 90.72% when pulling c688d03 on updateTally into 6b9a93d on master.

coveralls · 2017-02-24T20:31:28Z

Coverage decreased (-0.3%) to 90.751% when pulling c688d03 on updateTally into 6b9a93d on master.

coveralls · 2017-02-24T20:31:28Z

Coverage decreased (-0.3%) to 90.751% when pulling c688d03 on updateTally into 6b9a93d on master.

ascandella

lgtm, would like a once-over from @glibsm or somebody else familiar with our tally stuff

ascandella · 2017-02-27T17:40:51Z

metrics/nop_reporter.go

@@ -99,3 +103,30 @@ type nopCachedTimer struct {

 func (nopCachedTimer) ReportTimer(interval time.Duration) {
 }
+
+// NopCachedHistogram is an implementation of tally.CachedHistogram


maybe mention this is exported for testing?

Should we do this everywhere in this file? I'd prefer that, but not sure, if it will confuse reviewers.

ascandella · 2017-02-27T17:41:18Z

metrics/nop_reporter.go

+// NopCachedHistogram is an implementation of tally.CachedHistogram
+var NopCachedHistogram tally.CachedHistogram = nopCachedHistogram{}
+
+type nopCachedHistogram struct {


style nit: empty struct on a single line generally, right? or are we doing differerently:

type nopCachedHistogram struct{}

Well, I was following the leader(couple lines above) :)

ascandella · 2017-02-27T17:42:43Z

metrics/nop_reporter.go

+}
+
+func (nopCachedHistogram) ValueBucket(
+	bucketLowerBound, bucketUpperBound float64,


style nit: inconsistent oneline versus multiline. we may need a style guide :|

i thought we either did:

func foo(lotsAndLots, ofArgumentsHere ofThisType) {

or

func foo( lotsAndLots, ofArguments ofThisType, )

but maybe i'm misremembering

They have the same type, but sure.

ascandella · 2017-02-27T17:43:44Z

metrics/nop_reporter.go

@@ -62,6 +62,10 @@ func (nopCachedStatsReporter) AllocateGauge(name string, tags map[string]string)
 	return NopCachedGauge
 }

+func (nopCachedStatsReporter) AllocateHistogram(name string, tags map[string]string, buckets tally.Buckets) tally.CachedHistogram {


how come this one doesn't get godoc comments but the other one you added does?

nopCachedStatsReporter is the internal type.

ascandella · 2017-02-27T17:44:58Z

service/service_test.go

@@ -38,7 +38,16 @@ import (
 func TestServiceCreation(t *testing.T) {
 	r := metrics.NewTestStatsReporter()
 	r.CountersWG.Add(1)
-	scope, closer := tally.NewRootScope("", nil, r, 50*time.Millisecond, tally.DefaultSeparator)
+	opts := tally.ScopeOptions{
+		Tags:           nil,


why the explicit nil/empty string stuff here? i think i actually like it, but generally see it elided as:

opts := tally.ScopeOptions { Reporter: r, Separator: ..., DefaultBuckets: ..., }

for brevity. thoughts?

Well, I was afraid linter would complain, because, we can extend a struct and forget some mandatory fields. But it didn't. Let's get dangerous.

ascandella · 2017-02-27T17:45:31Z

testutils/metrics/metrics.go

+		DefaultBuckets: tally.DefaultBuckets,
+	}
+
+	scope, _ := tally.NewRootScope(opts, 100*time.Millisecond)


unrelated to this change, but what is this magic number for?

It is the reporting interval, how often we are going to sample metrics.

anuptalwalkar · 2017-02-27T18:16:31Z

testutils/metrics/metrics.go

+	Counters           map[string]int64
+	Gauges             map[string]float64
+	Timers             map[string]time.Duration
+	HistogramSamples   map[string]tally.Buckets


Has this been implemented internally? If not we cannot use it until the internal implementation support these APIs.

Why? Nop implementations doesn't have to do anything.

This is needed only to implement interfaces.

The way service is using it is in service_setup.go, where options became a separate struct.

I meant for Tally 2.0 CachedStatsReporter. We don't have its implementation here, but we rely on internal implementation from our private repo.

ascandella · 2017-02-27T19:26:30Z

metrics/nop_reporter.go


 func (nopCachedHistogram) ValueBucket(
-	bucketLowerBound, bucketUpperBound float64,
+	bucketLowerBound float64,


note: i wasn't suggesting repeating the type, just putting each param on its own line or having them all on one line, not some mix-and-match

I know, personally, it is easier to read. Really wish go fmt will help with these things :)

coveralls · 2017-02-27T19:32:49Z

Coverage decreased (-0.2%) to 90.875% when pulling 6769cd1 on updateTally into 6b9a93d on master.

glibsm · 2017-02-27T21:02:37Z

Looks good. What Anup said about internal implementation changes applies. As soon as this lands, m3 backend will be broken.

coveralls · 2017-03-07T19:53:26Z

Coverage decreased (-0.2%) to 91.509% when pulling fb2ffe2 on updateTally into be1e111 on master.

coveralls · 2017-03-07T20:14:10Z

Coverage decreased (-0.2%) to 91.509% when pulling d303ea8 on updateTally into be1e111 on master.

anuptalwalkar · 2017-03-08T01:27:13Z

merging since we have all the necessary components ready.

ascandella requested a review from glibsm February 27, 2017 17:40

ascandella approved these changes Feb 27, 2017

View reviewed changes

anuptalwalkar reviewed Feb 27, 2017

View reviewed changes

anuptalwalkar mentioned this pull request Feb 27, 2017

NopCachedStatsReporter uber-go/tally#23

Open

anuptalwalkar approved these changes Feb 27, 2017

View reviewed changes

ghost changed the title ~~Consume latest tally~~ [Do not merge]Consume latest tally Feb 27, 2017

ascandella reviewed Feb 27, 2017

View reviewed changes

glibsm approved these changes Feb 27, 2017

View reviewed changes

anuptalwalkar and others added 3 commits March 7, 2017 11:51

rebase from master

40d2fb9

Remove extra line

e8a08b0

Address comments

d303ea8

anuptalwalkar force-pushed the updateTally branch from fb2ffe2 to d303ea8 Compare March 7, 2017 20:09

anuptalwalkar merged commit 2470d0e into master Mar 8, 2017

anuptalwalkar deleted the updateTally branch March 8, 2017 01:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Do not merge]Consume latest tally #334

[Do not merge]Consume latest tally #334

ghost commented Feb 24, 2017 •

edited by ghost

Loading

mention-bot commented Feb 24, 2017

coveralls commented Feb 24, 2017

coveralls commented Feb 24, 2017

coveralls commented Feb 24, 2017

ascandella left a comment

ascandella Feb 27, 2017

ghost Feb 27, 2017

ascandella Feb 27, 2017

ghost Feb 27, 2017

ascandella Feb 27, 2017

ghost Feb 27, 2017

ascandella Feb 27, 2017

ghost Feb 27, 2017

ascandella Feb 27, 2017

ghost Feb 27, 2017

ascandella Feb 27, 2017

ghost Feb 27, 2017

anuptalwalkar Feb 27, 2017

ghost Feb 27, 2017

ghost Feb 27, 2017

anuptalwalkar Feb 27, 2017

ascandella Feb 27, 2017

ghost Feb 27, 2017

coveralls commented Feb 27, 2017

glibsm commented Feb 27, 2017

coveralls commented Mar 7, 2017

coveralls commented Mar 7, 2017

anuptalwalkar commented Mar 8, 2017

[Do not merge]Consume latest tally #334

[Do not merge]Consume latest tally #334

Conversation

ghost commented Feb 24, 2017 • edited by ghost Loading

mention-bot commented Feb 24, 2017

coveralls commented Feb 24, 2017

coveralls commented Feb 24, 2017

coveralls commented Feb 24, 2017

ascandella left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 27, 2017

glibsm commented Feb 27, 2017

coveralls commented Mar 7, 2017

coveralls commented Mar 7, 2017

anuptalwalkar commented Mar 8, 2017

ghost commented Feb 24, 2017 •

edited by ghost

Loading