Skip to content

[py] Implement backward-compatible increment/decrement #623

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.

Sign up for GitHub

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

Merged
olivielpeau merged 1 commit into from
Sep 26, 2017
Merged

[py] Implement backward-compatible increment/decrement #623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions pkg/aggregator/sender.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ type Sender interface {
Rate(metric string, value float64, hostname string, tags []string)
Count(metric string, value float64, hostname string, tags []string)
MonotonicCount(metric string, value float64, hostname string, tags []string)
Counter(metric string, value float64, hostname string, tags []string)
Histogram(metric string, value float64, hostname string, tags []string)
Historate(metric string, value float64, hostname string, tags []string)
ServiceCheck(checkName string, status metrics.ServiceCheckStatus, hostname string, tags []string, message string)
Expand Down Expand Up @@ -197,6 +198,13 @@ func (s *checkSender) MonotonicCount(metric string, value float64, hostname stri
s.sendMetricSample(metric, value, hostname, tags, metrics.MonotonicCountType)
}

// Counter is DEPRECATED and only implemented to preserve backward compatibility with python checks. Prefer using either:
// * `Gauge` if you're counting states
// * `Count` if you're counting events
func (s *checkSender) Counter(metric string, value float64, hostname string, tags []string) {
s.sendMetricSample(metric, value, hostname, tags, metrics.CounterType)
}

// Histogram should be used to track the statistical distribution of a set of values during a check run
// Should be called multiple times on the same (metric, hostname, tags) so that a distribution can be computed
func (s *checkSender) Histogram(metric string, value float64, hostname string, tags []string) {
Expand Down
6 changes: 6 additions & 0 deletions pkg/aggregator/sender_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ func TestCheckSenderInterface(t *testing.T) {
checkSender.Rate("my.rate_metric", 2.0, "my-hostname", []string{"foo", "bar"})
checkSender.Count("my.count_metric", 123.0, "my-hostname", []string{"foo", "bar"})
checkSender.MonotonicCount("my.monotonic_count_metric", 12.0, "my-hostname", []string{"foo", "bar"})
checkSender.Counter("my.counter_metric", 1.0, "my-hostname", []string{"foo", "bar"})
checkSender.Histogram("my.histo_metric", 3.0, "my-hostname", []string{"foo", "bar"})
checkSender.Commit()
checkSender.ServiceCheck("my_service.can_connect", metrics.ServiceCheckOK, "my-hostname", []string{"foo", "bar"}, "message")
Expand Down Expand Up @@ -163,6 +164,11 @@ func TestCheckSenderInterface(t *testing.T) {
assert.Equal(t, metrics.MonotonicCountType, monotonicCountSenderSample.metricSample.Mtype)
assert.Equal(t, false, monotonicCountSenderSample.commit)

CounterSenderSample := <-senderMetricSampleChan
assert.EqualValues(t, checkID1, CounterSenderSample.id)
assert.Equal(t, metrics.CounterType, CounterSenderSample.metricSample.Mtype)
assert.Equal(t, false, CounterSenderSample.commit)

histoSenderSample := <-senderMetricSampleChan
assert.EqualValues(t, checkID1, histoSenderSample.id)
assert.Equal(t, metrics.HistogramType, histoSenderSample.metricSample.Mtype)
Expand Down
5 changes: 5 additions & 0 deletions pkg/collector/corechecks/network/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ func (m *MockSender) MonotonicCount(metric string, value float64, hostname strin
m.Called(metric, value, hostname, tags)
}

//Counter adds a counter type to the mock calls.
func (m *MockSender) Counter(metric string, value float64, hostname string, tags []string) {
m.Called(metric, value, hostname, tags)
}

//Histogram adds a histogram type to the mock calls.
func (m *MockSender) Histogram(metric string, value float64, hostname string, tags []string) {
m.Called(metric, value, hostname, tags)
Expand Down
5 changes: 5 additions & 0 deletions pkg/collector/corechecks/system/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ func (m *MockSender) MonotonicCount(metric string, value float64, hostname strin
m.Called(metric, value, hostname, tags)
}

//Counter adds a counter type to the mock calls.
func (m *MockSender) Counter(metric string, value float64, hostname string, tags []string) {
m.Called(metric, value, hostname, tags)
}

//Histogram adds a histogram type to the mock calls.
func (m *MockSender) Histogram(metric string, value float64, hostname string, tags []string) {
m.Called(metric, value, hostname, tags)
Expand Down
8 changes: 4 additions & 4 deletions pkg/collector/dist/checks/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ def __init__(self, *args, **kwargs):
self._deprecations = {
'increment': [
Copy link
Copy Markdown
Contributor

@gmmeyer gmmeyer Sep 26, 2017

Choose a reason for hiding this comment

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

Can you add decrement here too?

Copy link
Copy Markdown
Member Author

@olivielpeau olivielpeau Sep 26, 2017

Choose a reason for hiding this comment

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

actually here we log the deprecation warning once per check instance, whether increment or decrement is used in the check. Which is why there's only one entry in this dict.

The key in this dict could be renamed though (to counter maybe?) so that this behavior is more self-explanatory.

Let me know what you think, I'll go ahead and merge this but I can address your comments in a separate PR :)

False,
"DEPRECATION NOTICE: `AgentCheck.increment`/`AgentCheck.decrement` are deprecated, sending these " +
"metrics with `AgentCheck.count` and a '_count' suffix instead",
"DEPRECATION NOTICE: `AgentCheck.increment`/`AgentCheck.decrement` are deprecated, please use " +
"`AgentCheck.gauge` or `AgentCheck.count` instead, with a different metric name",
],
'device_name': [
False,
Expand Down Expand Up @@ -132,11 +132,11 @@ def historate(self, name, value, tags=None, hostname=None, device_name=None):

def increment(self, name, value=1, tags=None, hostname=None, device_name=None):
self._log_deprecation("increment")
self.count(name + "_count", value, tags=tags, hostname=hostname, device_name=device_name)
self._submit_metric(aggregator.COUNTER, name, value, tags=tags, hostname=hostname, device_name=device_name)

def decrement(self, name, value=-1, tags=None, hostname=None, device_name=None):
self._log_deprecation("increment")
self.count(name + "_count", value, tags=tags, hostname=hostname, device_name=device_name)
self._submit_metric(aggregator.COUNTER, name, value, tags=tags, hostname=hostname, device_name=device_name)

def _log_deprecation(self, deprecation_key):
"""
Expand Down
1 change: 1 addition & 0 deletions pkg/collector/py/api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ char* MetricTypeNames[] = {
"RATE",
"COUNT",
"MONOTONIC_COUNT",
"COUNTER",
"HISTOGRAM",
"HISTORATE"
};
Expand Down
2 changes: 2 additions & 0 deletions pkg/collector/py/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ func SubmitMetric(check *C.PyObject, checkID *C.char, mt C.MetricType, name *C.c
sender.Count(_name, _value, _hostname, _tags)
case C.MONOTONIC_COUNT:
sender.MonotonicCount(_name, _value, _hostname, _tags)
case C.COUNTER:
sender.Counter(_name, _value, _hostname, _tags)
case C.HISTOGRAM:
sender.Histogram(_name, _value, _hostname, _tags)
case C.HISTORATE:
Expand Down
1 change: 1 addition & 0 deletions pkg/collector/py/api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ typedef enum {
RATE,
COUNT,
MONOTONIC_COUNT,
COUNTER,
HISTOGRAM,
HISTORATE,
MT_LAST = HISTORATE
Expand Down
4 changes: 4 additions & 0 deletions pkg/collector/py/check_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,8 @@ func TestAggregatorLink(t *testing.T) {
[]string{"foo", "bar"}, "a message").Return().Times(1)
mockSender.On("Gauge", "testmetric", mock.AnythingOfType("float64"), "", []string(nil)).Return().Times(1)
mockSender.On("Gauge", "testmetricstringvalue", mock.AnythingOfType("float64"), "", []string(nil)).Return().Times(1)
mockSender.On("Counter", "test.increment", 1., "", []string{"foo", "bar"}).Return().Times(1)
mockSender.On("Counter", "test.decrement", -1., "", []string{"foo", "bar", "baz"}).Return().Times(1)
mockSender.On("Event", mock.AnythingOfType("metrics.Event")).Return().Times(1)
mockSender.On("Commit").Return().Times(1)

Expand All @@ -166,6 +168,8 @@ func TestAggregatorLinkTwoRuns(t *testing.T) {
[]string{"foo", "bar"}, "a message").Return().Times(2)
mockSender.On("Gauge", "testmetric", mock.AnythingOfType("float64"), "", []string(nil)).Return().Times(2)
mockSender.On("Gauge", "testmetricstringvalue", mock.AnythingOfType("float64"), "", []string(nil)).Return().Times(2)
mockSender.On("Counter", "test.increment", 1., "", []string{"foo", "bar"}).Return().Times(2)
mockSender.On("Counter", "test.decrement", -1., "", []string{"foo", "bar", "baz"}).Return().Times(2)
mockSender.On("Event", mock.AnythingOfType("metrics.Event")).Return().Times(2)
mockSender.On("Commit").Return().Times(2)

Expand Down
5 changes: 5 additions & 0 deletions pkg/collector/py/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ func (m *MockSender) MonotonicCount(metric string, value float64, hostname strin
m.Called(metric, value, hostname, tags)
}

//Counter adds a counter type to the mock calls.
func (m *MockSender) Counter(metric string, value float64, hostname string, tags []string) {
m.Called(metric, value, hostname, tags)
}

//Histogram adds a histogram type to the mock calls.
func (m *MockSender) Histogram(metric string, value float64, hostname string, tags []string) {
m.Called(metric, value, hostname, tags)
Expand Down
3 changes: 3 additions & 0 deletions pkg/collector/py/tests/testaggregator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ def check(self, instance):
else:
raise Exception("Expected gauge to raise ValueError")

self.increment("test.increment", tags=['foo', 'bar'])
self.decrement("test.decrement", tags=['foo', 'bar', 'baz'])

self.event({
"event_type": "new.event",
"msg_title": "new test event",
Expand Down