From 75a444ce440acb3b09afbbaeacdf1055bb29614c Mon Sep 17 00:00:00 2001 From: Remy Mathieu Date: Mon, 27 Mar 2023 15:05:50 +0200 Subject: [PATCH 1/3] Validate value type on metric submission. --- lib/datadog/statsd.rb | 33 +++++++++++++++++++++++++++++---- spec/statsd_spec.rb | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/lib/datadog/statsd.rb b/lib/datadog/statsd.rb index 034769cd..181db00f 100644 --- a/lib/datadog/statsd.rb +++ b/lib/datadog/statsd.rb @@ -108,6 +108,7 @@ def initialize( raise ArgumentError, 'tags must be an array of string tags or a Hash' end + @logger = logger @namespace = namespace @prefix = @namespace ? "#{@namespace}.".freeze : nil @serializer = Serialization::Serializer.new(prefix: @prefix, global_tags: tags) @@ -187,6 +188,10 @@ def decrement(stat, opts = EMPTY_OPTIONS) # @option opts [Array] :tags An array of tags def count(stat, count, opts = EMPTY_OPTIONS) opts = { sample_rate: opts } if opts.is_a?(Numeric) + if not count.is_a?(Numeric) + @logger.error("count: count value should be numeric") if @logger + return + end send_stats(stat, count, COUNTER_TYPE, opts) end @@ -206,6 +211,10 @@ def count(stat, count, opts = EMPTY_OPTIONS) # $statsd.gauge('user.count', User.count) def gauge(stat, value, opts = EMPTY_OPTIONS) opts = { sample_rate: opts } if opts.is_a?(Numeric) + if not value.is_a?(Numeric) + @logger.error("gauge: value should be numeric") if @logger + return + end send_stats(stat, value, GAUGE_TYPE, opts) end @@ -220,6 +229,10 @@ def gauge(stat, value, opts = EMPTY_OPTIONS) # @example Report the current user count: # $statsd.histogram('user.count', User.count) def histogram(stat, value, opts = EMPTY_OPTIONS) + if not value.is_a?(Numeric) + @logger.error("histogram: value should be numeric") if @logger + return + end send_stats(stat, value, HISTOGRAM_TYPE, opts) end @@ -234,6 +247,10 @@ def histogram(stat, value, opts = EMPTY_OPTIONS) # @example Report the current user count: # $statsd.distribution('user.count', User.count) def distribution(stat, value, opts = EMPTY_OPTIONS) + if not value.is_a?(Numeric) + @logger.error("distribution: value should be numeric") if @logger + return + end send_stats(stat, value, DISTRIBUTION_TYPE, opts) end @@ -270,6 +287,10 @@ def distribution_time(stat, opts = EMPTY_OPTIONS) # @option opts [Array] :tags An array of tags def timing(stat, ms, opts = EMPTY_OPTIONS) opts = { sample_rate: opts } if opts.is_a?(Numeric) + if not ms.is_a?(Numeric) + @logger.error("timing: ms should be numeric") if @logger + return + end send_stats(stat, ms, TIMING_TYPE, opts) end @@ -307,6 +328,10 @@ def time(stat, opts = EMPTY_OPTIONS) # $statsd.set('visitors.uniques', User.id) def set(stat, value, opts = EMPTY_OPTIONS) opts = { sample_rate: opts } if opts.is_a?(Numeric) + if not value.is_a?(Numeric) + @logger.error("set: value should be numeric") if @logger + return + end send_stats(stat, value, SET_TYPE, opts) end @@ -315,10 +340,10 @@ def set(stat, value, opts = EMPTY_OPTIONS) # @param [String] name Service check name # @param [String] status Service check status. # @param [Hash] opts the additional data about the service check - # @option opts [Integer, String, nil] :timestamp (nil) Assign a timestamp to the service check. Default is now when none - # @option opts [String, nil] :hostname (nil) Assign a hostname to the service check. - # @option opts [Array, nil] :tags (nil) An array of tags - # @option opts [String, nil] :message (nil) A message to associate with this service check status + # @option opts [Integer, String, nil] :timestamp (nil) Assign a timestamp to the service check. Default is now when none + # @option opts [String, nil] :hostname (nil) Assign a hostname to the service check. + # @option opts [Array, nil] :tags (nil) An array of tags + # @option opts [String, nil] :message (nil) A message to associate with this service check status # @example Report a critical service check status # $statsd.service_check('my.service.check', Statsd::CRITICAL, :tags=>['urgent']) def service_check(name, status, opts = EMPTY_OPTIONS) diff --git a/spec/statsd_spec.rb b/spec/statsd_spec.rb index 1c659b21..de670290 100644 --- a/spec/statsd_spec.rb +++ b/spec/statsd_spec.rb @@ -261,6 +261,48 @@ expect(socket.recv[0]).to eq_with_telemetry('foobar:1|c') end + it 'doesnt send count when the value isnt numeric' do + subject.count('metric_name', 'hello') + subject.flush(sync: true) + + expect(socket.recv).to be nil + end + + it 'doesnt send gauge when the value isnt numeric' do + subject.gauge('metric_name', 'hello') + subject.flush(sync: true) + + expect(socket.recv).to be nil + end + + it 'doesnt send set when the value isnt numeric' do + subject.set('metric_name', 'hello') + subject.flush(sync: true) + + expect(socket.recv).to be nil + end + + it 'doesnt send distribution when the value isnt numeric' do + subject.distribution('metric_name', 'hello') + subject.flush(sync: true) + + expect(socket.recv).to be nil + end + + it 'doesnt send histogram when the value isnt numeric' do + subject.histogram('metric_name', 'hello') + subject.flush(sync: true) + + expect(socket.recv).to be nil + end + + it 'doesnt send timing when the value isnt numeric' do + subject.timing('metric_name', 'hello') + subject.flush(sync: true) + + expect(socket.recv).to be nil + end + context 'with a sample rate' do before do allow(subject).to receive(:rand).and_return(0) From 9a475f242b04cc1eb5a705f5a11b565ce761728f Mon Sep 17 00:00:00 2001 From: Remy Mathieu Date: Mon, 3 Apr 2023 17:59:51 +0200 Subject: [PATCH 2/3] Use the more idiomatic `unless`. --- lib/datadog/statsd.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/datadog/statsd.rb b/lib/datadog/statsd.rb index 181db00f..528223da 100644 --- a/lib/datadog/statsd.rb +++ b/lib/datadog/statsd.rb @@ -188,7 +188,7 @@ def decrement(stat, opts = EMPTY_OPTIONS) # @option opts [Array] :tags An array of tags def count(stat, count, opts = EMPTY_OPTIONS) opts = { sample_rate: opts } if opts.is_a?(Numeric) - if not count.is_a?(Numeric) + unless count.is_a?(Numeric) @logger.error("count: count value should be numeric") if @logger return end @@ -211,7 +211,7 @@ def count(stat, count, opts = EMPTY_OPTIONS) # $statsd.gauge('user.count', User.count) def gauge(stat, value, opts = EMPTY_OPTIONS) opts = { sample_rate: opts } if opts.is_a?(Numeric) - if not value.is_a?(Numeric) + unless value.is_a?(Numeric) @logger.error("gauge: value should be numeric") if @logger return end @@ -229,7 +229,7 @@ def gauge(stat, value, opts = EMPTY_OPTIONS) # @example Report the current user count: # $statsd.histogram('user.count', User.count) def histogram(stat, value, opts = EMPTY_OPTIONS) - if not value.is_a?(Numeric) + unless value.is_a?(Numeric) @logger.error("histogram: value should be numeric") if @logger return end @@ -247,7 +247,7 @@ def histogram(stat, value, opts = EMPTY_OPTIONS) # @example Report the current user count: # $statsd.distribution('user.count', User.count) def distribution(stat, value, opts = EMPTY_OPTIONS) - if not value.is_a?(Numeric) + unless value.is_a?(Numeric) @logger.error("distribution: value should be numeric") if @logger return end @@ -287,7 +287,7 @@ def distribution_time(stat, opts = EMPTY_OPTIONS) # @option opts [Array] :tags An array of tags def timing(stat, ms, opts = EMPTY_OPTIONS) opts = { sample_rate: opts } if opts.is_a?(Numeric) - if not ms.is_a?(Numeric) + unless ms.is_a?(Numeric) @logger.error("timing: ms should be numeric") if @logger return end @@ -328,7 +328,7 @@ def time(stat, opts = EMPTY_OPTIONS) # $statsd.set('visitors.uniques', User.id) def set(stat, value, opts = EMPTY_OPTIONS) opts = { sample_rate: opts } if opts.is_a?(Numeric) - if not value.is_a?(Numeric) + unless value.is_a?(Numeric) @logger.error("set: value should be numeric") if @logger return end From 0e9b9585da25651dafec2c2e4d6548ed1d4f8b31 Mon Sep 17 00:00:00 2001 From: Remy Mathieu Date: Thu, 6 Apr 2023 17:22:08 +0200 Subject: [PATCH 3/3] Set string value are a thing. --- lib/datadog/statsd.rb | 11 ++++++++--- spec/statsd_spec.rb | 23 +++++++++++++++-------- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/lib/datadog/statsd.rb b/lib/datadog/statsd.rb index 528223da..ee34b0d7 100644 --- a/lib/datadog/statsd.rb +++ b/lib/datadog/statsd.rb @@ -328,9 +328,8 @@ def time(stat, opts = EMPTY_OPTIONS) # $statsd.set('visitors.uniques', User.id) def set(stat, value, opts = EMPTY_OPTIONS) opts = { sample_rate: opts } if opts.is_a?(Numeric) - unless value.is_a?(Numeric) - @logger.error("set: value should be numeric") if @logger - return + if value.is_a?(String) + value = escape(value) end send_stats(stat, value, SET_TYPE, opts) end @@ -444,6 +443,12 @@ def now Process.clock_gettime(Process::CLOCK_MONOTONIC) end + def escape(text) + text.delete('|').tap do |t| + t.gsub!("\n", '\n') + end + end + def send_stats(stat, delta, type, opts = EMPTY_OPTIONS) telemetry.sent(metrics: 1) if telemetry diff --git a/spec/statsd_spec.rb b/spec/statsd_spec.rb index de670290..4c6f0895 100644 --- a/spec/statsd_spec.rb +++ b/spec/statsd_spec.rb @@ -261,42 +261,49 @@ expect(socket.recv[0]).to eq_with_telemetry('foobar:1|c') end - it 'doesnt send count when the value isnt numeric' do + it 'does not send count when the value isnt numeric' do subject.count('metric_name', 'hello') subject.flush(sync: true) expect(socket.recv).to be nil end - it 'doesnt send gauge when the value isnt numeric' do + it 'does not send gauge when the value isnt numeric' do subject.gauge('metric_name', 'hello') subject.flush(sync: true) expect(socket.recv).to be nil end - it 'doesnt send set when the value isnt numeric' do - subject.set('metric_name', 'hello') + it 'escape set when the value is a string' do + subject.set('metric_name', "hel\nl|o") subject.flush(sync: true) - expect(socket.recv).to be nil + expect(socket.recv[0]).to eq_with_telemetry("metric_name:hel\\nlo|s") + end + + it 'does not escape set when the value is numeric' do + subject.set('metric_name', 5) + subject.flush(sync: true) + + expect(socket.recv[0]).to eq_with_telemetry("metric_name:5|s") end - it 'doesnt send distribution when the value isnt numeric' do + it 'does not send distribution when the value isnt numeric' do subject.distribution('metric_name', 'hello') subject.flush(sync: true) expect(socket.recv).to be nil end - it 'doesnt send histogram when the value isnt numeric' do + it 'does not send histogram when the value isnt numeric' do subject.histogram('metric_name', 'hello') subject.flush(sync: true) expect(socket.recv).to be nil end - it 'doesnt send timing when the value isnt numeric' do + it 'does not send timing when the value isnt numeric' do subject.timing('metric_name', 'hello') subject.flush(sync: true)