Skip to content

Commit 908ee3d

Browse files
authored
Adds handling for malformed metrics (#337)
* Adds handling for malformed metrics In the Ruby GraphQL gem we have observed some interesting behavior where collectors appear not to be registered, which causes the code from that gem to error: ```ruby @trace.prometheus_client.send_json( # [!!] No name here type: @trace.prometheus_collector_type, duration: duration, platform_key: event_name, key: keyword ) ``` On one hand this appears to be a malformed request body as-is, and the tests do not appear to test the integration in validating this behavior. Given that, that brings up a potential edge case which may warrant some discussion: What should PrometheusExporter do in a case where it gets a malformed metric that does not have a name? Should it give an error? Right now what it returns is this: ```ruby ``` ...which does not give clear tracability into the cause and potential resolutions. The opinion of this PR, at least, is to allow it to gracefully fail when a metric cannot be registered but I am not convinced that this is the correct behavior in this scenario and would defer to the folks working on the project as to what makes the most sense here. * Fix logger calls in Collector by passing WebServer logger - Add logger parameter to Collector#initialize with default fallback - Pass WebServer's @logger to Collector during initialization - Add tests for logger functionality and malformed metric warnings
1 parent 71d55d5 commit 908ee3d

File tree

3 files changed

+55
-3
lines changed

3 files changed

+55
-3
lines changed

lib/prometheus_exporter/server/collector.rb

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,13 @@
11
# frozen_string_literal: true
22

3+
require "logger"
4+
35
module PrometheusExporter::Server
46
class Collector < CollectorBase
5-
def initialize(json_serializer: nil)
7+
attr_reader :logger
8+
9+
def initialize(json_serializer: nil, logger: Logger.new(STDERR))
10+
@logger = logger
611
@process_metrics = []
712
@metrics = {}
813
@mutex = Mutex.new
@@ -40,6 +45,8 @@ def process_hash(obj)
4045
metric = @metrics[obj["name"]]
4146
metric = register_metric_unsafe(obj) if !metric
4247

48+
next unless metric
49+
4350
keys = obj["keys"] || {}
4451
keys = obj["custom_labels"].merge(keys) if obj["custom_labels"]
4552

@@ -74,6 +81,11 @@ def register_metric_unsafe(obj)
7481
help = obj["help"]
7582
opts = symbolize_keys(obj["opts"] || {})
7683

84+
if !name
85+
logger.warn "failed to register metric due to empty name #{obj}"
86+
return
87+
end
88+
7789
metric =
7890
case obj["type"]
7991
when "gauge"
@@ -89,7 +101,7 @@ def register_metric_unsafe(obj)
89101
if metric
90102
@metrics[name] = metric
91103
else
92-
STDERR.puts "failed to register metric #{obj}"
104+
logger.warn "failed to register metric #{obj}"
93105
end
94106
end
95107

lib/prometheus_exporter/server/web_server.rb

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ class WebServer
1212
def initialize(opts)
1313
@port = opts[:port] || PrometheusExporter::DEFAULT_PORT
1414
@bind = opts[:bind] || PrometheusExporter::DEFAULT_BIND_ADDRESS
15-
@collector = opts[:collector] || Collector.new
1615
@timeout = opts[:timeout] || PrometheusExporter::DEFAULT_TIMEOUT
1716
@verbose = opts[:verbose] || false
1817
@auth = opts[:auth]
@@ -61,6 +60,8 @@ def initialize(opts)
6160
@bind = nil
6261
end
6362

63+
@collector = opts[:collector] || Collector.new(logger: @logger)
64+
6465
@server =
6566
WEBrick::HTTPServer.new(
6667
Port: @port,

test/server/collector_test.rb

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,45 @@ def test_register_metric
100100
assert_equal(text, collector.prometheus_metrics_text)
101101
end
102102

103+
def test_malformed_metric_without_name_or_action
104+
collector = PrometheusExporter::Server::Collector.new
105+
106+
json = {
107+
type: :gauge,
108+
keys: {
109+
key1: "test1",
110+
},
111+
value: 1,
112+
}.to_json
113+
114+
collector.process(json)
115+
116+
assert_equal("", collector.prometheus_metrics_text)
117+
end
118+
119+
def test_default_logger
120+
collector = PrometheusExporter::Server::Collector.new
121+
assert_instance_of(Logger, collector.logger)
122+
end
123+
124+
def test_custom_logger
125+
logs = StringIO.new
126+
custom_logger = Logger.new(logs)
127+
collector = PrometheusExporter::Server::Collector.new(logger: custom_logger)
128+
129+
assert_equal(custom_logger, collector.logger)
130+
end
131+
132+
def test_logger_warns_on_malformed_metric
133+
logs = StringIO.new
134+
collector = PrometheusExporter::Server::Collector.new(logger: Logger.new(logs))
135+
136+
json = { type: :gauge, keys: { key1: "test1" }, value: 1 }.to_json
137+
collector.process(json)
138+
139+
assert_includes(logs.string, "failed to register metric due to empty name")
140+
end
141+
103142
def test_it_can_increment_gauge_when_specified
104143
name = "test_name"
105144
help = "test_help"

0 commit comments

Comments
 (0)