Skip to content
This repository was archived by the owner on Aug 17, 2025. It is now read-only.

Conversation

black-adder
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Oct 17, 2016

Coverage Status

Coverage decreased (-35.2%) to 64.807% when pulling 1da622b on add_metrics into 321ba1b on master.

@black-adder
Copy link
Contributor Author

welllll, it looks like I missed a spot

@coveralls
Copy link

coveralls commented Oct 17, 2016

Coverage Status

Coverage decreased (-15.5%) to 84.549% when pulling 8fe296b on add_metrics into 321ba1b on master.

@coveralls
Copy link

coveralls commented Oct 17, 2016

Coverage Status

Coverage decreased (-10.7%) to 89.27% when pulling 916fa89 on add_metrics into 321ba1b on master.

@yurishkuro
Copy link
Member

some easy wins for coverage is to exercise the noop impls. But the biggest unexercised chunk is reading from the timer channel. If you run it with high frequency, like 1ms, then you can also test it.

@yurishkuro
Copy link
Member

btw, the actual build error is not about coverage (it does not block PRs), but from a test error

=== RUN   TestLocalMetrics
--- FAIL: TestLocalMetrics (0.00s)
    Error Trace:    local_test.go:15
            local_test.go:75
    Error:      Not equal: 3 (expected)
                    != 4 (actual)
    Messages:   Leaked at least one goroutine.

The class should have proper shutdown procedure when used in tests.

@@ -0,0 +1,14 @@
package metrics
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove pkg, this whole repo is the equivalent of pkg namespace.

@yurishkuro yurishkuro changed the title add metrics Add metrics Oct 18, 2016
@yurishkuro
Copy link
Member

yurishkuro commented Oct 18, 2016

Please read up on good commit messages - http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

I updated the PR title

@coveralls
Copy link

coveralls commented Oct 18, 2016

Coverage Status

Coverage decreased (-3.4%) to 96.61% when pulling 3c48f28 on add_metrics into 321ba1b on master.

metrics/local.go Outdated
}

// NewLocalBackend returns a new LocalBackend
func NewLocalBackend(tick time.Duration) *LocalBackend {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/tick/collectionInterval/ and explain it in the doc string

metrics/local.go Outdated
stopped: make(chan struct{}),
}

go func() {
Copy link
Member

@yurishkuro yurishkuro Oct 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move to a function, e.g. runLoop()


// RecordTimer records a timing duration
func (b *LocalBackend) RecordTimer(name string, tags map[string]string, d time.Duration) {
timer := b.findOrCreateTimer(name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tags are ignored here. That will make it impossible to use this local backend in unit tests that are sensitive to the tags. We can address this in a different PR, basically by copying functionality from the metrics implementation in jaeger-client-go.

@coveralls
Copy link

coveralls commented Oct 18, 2016

Coverage Status

Coverage decreased (-3.4%) to 96.624% when pulling 472809b on add_metrics into 321ba1b on master.

@black-adder black-adder merged commit 7b113fc into master Oct 18, 2016
@yurishkuro yurishkuro deleted the add_metrics branch August 22, 2017 18:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants