-
Notifications
You must be signed in to change notification settings - Fork 304
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.
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
Support passing Unix timestamps to dogstatsd #831
Changes from all commits
89f678d
8eb39f5
2e51a33
88ab0a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -304,10 +304,31 @@ def test_set(self): | |
self.statsd.set('set', 123) | ||
self.assert_equal_telemetry('set:123|s\n', self.recv(2)) | ||
|
||
def test_report(self): | ||
self.statsd._report('report', 'g', 123.4, tags=None, sample_rate=None) | ||
self.assert_equal_telemetry('report:123.4|g\n', self.recv(2)) | ||
|
||
def test_report_metric_with_unsupported_ts(self): | ||
self.statsd._reset_telemetry() | ||
self.statsd._report('report', 'h', 123.5, tags=None, sample_rate=None, timestamp=100) | ||
self.assert_equal_telemetry('report:123.5|h\n', self.recv(2)) | ||
|
||
self.statsd._reset_telemetry() | ||
self.statsd._report('set', 's', 123, tags=None, sample_rate=None, timestamp=100) | ||
self.assert_equal_telemetry('set:123|s\n', self.recv(2)) | ||
|
||
def test_gauge(self): | ||
self.statsd.gauge('gauge', 123.4) | ||
self.assert_equal_telemetry('gauge:123.4|g\n', self.recv(2)) | ||
|
||
def test_gauge_with_ts(self): | ||
self.statsd.gauge_with_timestamp("gauge", 123.4, timestamp=1066) | ||
self.assert_equal_telemetry("gauge:123.4|g|T1066\n", self.recv(2)) | ||
|
||
def test_gauge_with_invalid_ts_should_be_ignored(self): | ||
self.statsd.gauge_with_timestamp("gauge", 123.4, timestamp=-500) | ||
self.assert_equal_telemetry("gauge:123.4|g\n", self.recv(2)) | ||
|
||
def test_counter(self): | ||
self.statsd.increment('page.views') | ||
self.statsd.flush() | ||
|
@@ -328,6 +349,26 @@ def test_counter(self): | |
self.statsd.flush() | ||
self.assert_equal_telemetry('page.views:-12|c\n', self.recv(2)) | ||
|
||
def test_count(self): | ||
self.statsd.count('page.views', 11) | ||
self.statsd.flush() | ||
self.assert_equal_telemetry('page.views:11|c\n', self.recv(2)) | ||
|
||
def test_count_with_ts(self): | ||
self.statsd.count_with_timestamp("page.views", 1, timestamp=1066) | ||
self.statsd.flush() | ||
self.assert_equal_telemetry("page.views:1|c|T1066\n", self.recv(2)) | ||
|
||
self.statsd._reset_telemetry() | ||
self.statsd.count_with_timestamp("page.views", 11, timestamp=2121) | ||
self.statsd.flush() | ||
self.assert_equal_telemetry("page.views:11|c|T2121\n", self.recv(2)) | ||
|
||
def test_count_with_invalid_ts_should_be_ignored(self): | ||
self.statsd.count_with_timestamp("page.views", 1, timestamp=-1066) | ||
self.statsd.flush() | ||
self.assert_equal_telemetry("page.views:1|c\n", self.recv(2)) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would that be worth a direct call to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I feel there should be one more test to make sure that if statement that checks metric types with timestamps is covered correctly. |
||
def test_histogram(self): | ||
self.statsd.histogram('histo', 123.4) | ||
self.assert_equal_telemetry('histo:123.4|h\n', self.recv(2)) | ||
|
@@ -518,7 +559,6 @@ def func(): | |
# check that the method does not fail with a small payload | ||
self.statsd.event("title", "message") | ||
|
||
|
||
def test_service_check(self): | ||
now = int(time.time()) | ||
self.statsd.service_check( | ||
|
@@ -1106,7 +1146,6 @@ def test_batching_sequential(self): | |
self.recv(2), | ||
telemetry=expected_metrics1) | ||
|
||
|
||
expected2 = 'page.views:123|g\ntimer:123|ms\n' | ||
self.assert_equal_telemetry( | ||
expected2, | ||
|
@@ -1276,7 +1315,6 @@ def test_telemetry(self): | |
self.statsd.bytes_dropped_queue = 8 | ||
self.statsd.packets_dropped_queue = 9 | ||
|
||
|
||
self.statsd.open_buffer() | ||
self.statsd.gauge('page.views', 123) | ||
self.statsd.close_buffer() | ||
|
@@ -1383,7 +1421,6 @@ def test_telemetry_flush_interval_batch(self): | |
# assert that _last_flush_time has been updated | ||
self.assertTrue(time1 < dogstatsd._last_flush_time) | ||
|
||
|
||
def test_dedicated_udp_telemetry_dest(self): | ||
listener_sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) | ||
listener_sock.bind(('localhost', 0)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think that would be worth a warning? It's a developer error at this point (somebody is calling
_report
with a wrongmetric_type
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes worth a test (as said in the other comment) but maybe we don't want to flood stdout with logs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test but not sure about adding a warning. In other SDKs we don't log a warning if invalid timestamp used. Here if they are trying to send a metric that doesn't support timestamps then they're accessing methods they shouldn't be in the first place.