Iotedge: Prometheus Regex doesn't match prometheus exposition format

[alaendle]

Unfortunately the RegEx doesn't handle the optional trailing commas - which are ugly but allowed:

https://github.com/Azure/iotedge/blob/6d81b94d413484e41243469e2430bff75ae514ee/edge-agent/src/Microsoft.Azure.Devices.Edge.Agent.Diagnostics/util/PrometheusMetricsParser.cs#L19-L24

Prometheus EBNF
https://prometheus.io/docs/instrumenting/exposition_formats/

metric_name [
  "{" label_name "=" `"` label_value `"` { "," label_name "=" `"` label_value `"` } [ "," ] "}"
] value [ timestamp ]

That means that a exposed metric like

edgeagent_total_disk_read_bytes{iothub=""lefitche-hub-3.azure-devices.net"",} 32908854

is absolutely valid. However the current parser RegEx doesn't match.

Please note that the Parser code seems to be duplicated in some projects - e.g. mcr.microsoft.com/azuremonitor/containerinsights/ciprod:iot-0.1.3.3

Please let me know if I was unclear or if you need further information.

[arsing]

Unfortunately the RegEx doesn't handle the optional trailing commas - which are ugly but allowed: [...] However the current parser RegEx doesn't match.

Yes, I agree. That docs page don't have any examples with a trailing comma, but the EBNF has clearly been intentionally written to allow it rather than as an accident, so we should fix the regex.

@lfitchett Do you foresee any problems? If not, I'll send a PR.

---

Please note that the Parser code seems to be duplicated in some projects - e.g. mcr.microsoft.com/azuremonitor/containerinsights/ciprod:iot-0.1.3.3

@daweim0 Since you submitted the regex initially in #3473, do you have the ability to fix the other places like Azure Monitor codebase as OP mentioned.

[daweim0]

~Hi, I can fix it in the azure monitor codebase. I don't have time to write a fix (ownership of that codebase is being transferred from azure monitor to IoT), but I can definitely pull in a fix available for iotedge (the prometheus parsers are identical)
@lfitchett ping me when a fix is available and I'll pull it into the azure monitor module?~

Actually I hear that the ownership handoff is happening soon, azure monitor will probably not push a fix

[arsing]

So I started fixing it, but there are a bunch of things wrong with this regex. I cross-referenced it with upstream's parser in https://github.com/prometheus/common/blob/master/expfmt/text_parse.go

1. There are various points where upstream's parser allows whitespace (specifically only spaces and tabs) between the tokens and ignores it, but the regex doesn't.

2. Upstream's parser allows \-escaped characters in tag values (label value), but the regex doesn't.

3. The regex seems to have a unique case to allow \. as a tag value, which I assume is a partial workaround for (2)

4. Upstream's parser allows a trailing comma between the last tag value and the }, but the regex doesn't. (The original issue here.)

5. Upstream's parser allows a timestamp after the metric value. Our code always uses UtcNow as the timestamp.

(2) especially is hard to parse with a regex because a tag value can contain an escaped ".

I feel like we ought to write a proper parser with state to mimic upstream's implementation. I'll follow up internally about this; I remember talk of an existing Prometheus library we could use the parser from, but that we can't use for some reason.

[alaendle]

@arsing, @daweim0 : Thank you for addressing the problem so quickly. However, for me as an end user it is of course still unsolved as long as no new artefacts are published. Will corresponding updates appear (soon)? You noticed that it is likely that there will be no update for azure monitor and I admit that this problem has a rather low priority, but the collection of prometheus metrics is an important property for connecting third-party software in the long term. So what should I be prepared for?

[veyalla]

@alaendle - this fix is targeted to make it into the metrics module in next 2-4week. FYI @damonbarry