Jimdo / prometheus_client_php

Prometheus instrumentation library for PHP applications
https://prometheus.io/docs/concepts/metric_types/
Apache License 2.0
281 stars 213 forks source link

Adds base64-encoding of label values to allow the usage of colons #57

Closed mps-sepetrov closed 6 years ago

mps-sepetrov commented 7 years ago

Expected behaviour

Label values can be any sequence of UTF-8 characters.

Current behaviour

No measures are taken to ensure that the label values can be persisted without affecting the retrieval process.

Steps to reproduce

  1. Persist a metric (counter, gauge, histogram) using APC or In-Memory storage, which has a label with value : (semicolon).
  2. Retrieve the metrics and observe PHP warnings and metrics with missing label.
<?php

require __DIR__ . '/vendor/autoload.php';

//$adapter = new Prometheus\Storage\APC();
//$adapter->flushAPC();

$adapter = new Prometheus\Storage\InMemory();
$adapter->flushMemory();

$registry = new Prometheus\CollectorRegistry($adapter);

$counter = $registry->registerCounter('test', 'some_counter', 'it increases', ['type']);
$counter->incBy(1, [':']);

$renderer = new Prometheus\RenderTextFormat();
$result = $renderer->render($registry->getMetricFamilySamples());

header('Content-type: ' . Prometheus\RenderTextFormat::MIME_TYPE);
echo $result;
//Warning: array_combine(): Both parameters should have an equal number of elements in ///var/www/html/src/Prometheus/RenderTextFormat.php on line 39
//
//Warning: Invalid argument supplied for foreach() in ///var/www/html/src/Prometheus/RenderTextFormat.php on line 40
//
//Warning: Cannot modify header information - headers already sent by (output started at ///var/www/html/src/Prometheus/RenderTextFormat.php:39) in /var/www/html/test.php on line 19
//# HELP test_some_counter it increases
//# TYPE test_some_counter counter
//test_some_counter{} 1

Proposed implementation

Suitable escaping strategy must be used, depending on the storage implementation. JSON-encoding is not enough to escape colons. Base64-encoding the already JSON-encoded values will make the string safe to persist when using APC and in-memory storage.

Unresolved issues

bracki commented 6 years ago

Nice bug! Instead of your proposed solution we should just reject : as an invalid label value. According to https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels the valid regex for labels is: [a-zA-Z_][a-zA-Z0-9_]*. So I'd suggest checking each the labels against this regex. The relevant code is here.

robbiet480 commented 6 years ago

@bracki FYI this issue affects label names as well as label values. Prometheus docs say that all Unicode characters are acceptable for values but only a certain set is acceptable for label names.

@mps-sepetrov This patch worked perfectly for me, thanks!

bracki commented 6 years ago

LGTM. Thank you for your contribution.