From 961c7a994dfaa3af51dafb6adca06de31c7c7e9d Mon Sep 17 00:00:00 2001 From: Juan Hoyos <19413848+hoyosjs@users.noreply.github.com> Date: Wed, 12 Aug 2026 22:41:36 -0700 Subject: [PATCH] Escape delimiters in metric event tags Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 30661229-7799-460e-b594-c6f27c79e3fe --- .../src/System/Diagnostics/Helpers.cs | 36 ++++- .../Diagnostics/Metrics/MetricsEventSource.cs | 24 ++-- .../MetricEventSourceTests.cs | 135 ++++++++++++++++++ 3 files changed, 185 insertions(+), 10 deletions(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Helpers.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Helpers.cs index 874b61de6958e4..764ab56ef202ae 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Helpers.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Helpers.cs @@ -10,6 +10,13 @@ namespace System.Diagnostics { internal static class Helpers { + // Tag lists are flattened into a single "key=value,key=value" string. Because tag keys + // and values are arbitrary strings that may themselves contain the ',' pair separator or + // the '=' key/value separator, each key and value is escaped so the string can be decoded + // without ambiguity. The escaping rules are: + // '\' => "\\" ',' => "\," '=' => "\=" + // The ',' between pairs and the '=' between a key and its value are emitted literally + // (unescaped) as delimiters. internal static string FormatTags(IEnumerable>? tags) { if (tags is null) @@ -30,7 +37,9 @@ internal static string FormatTags(IEnumerable>? ta sb.Append(','); } - sb.Append(tag.Key).Append('=').Append(tag.Value); + AppendEscaped(sb, tag.Key); + sb.Append('='); + AppendEscaped(sb, tag.Value?.ToString()); } return sb.ToString(); } @@ -45,7 +54,9 @@ internal static string FormatTags(KeyValuePair[] labels) StringBuilder sb = new StringBuilder(); for (int i = 0; i < labels.Length; i++) { - sb.Append(labels[i].Key).Append('=').Append(labels[i].Value); + AppendEscaped(sb, labels[i].Key); + sb.Append('='); + AppendEscaped(sb, labels[i].Value); if (i != labels.Length - 1) { sb.Append(','); @@ -54,6 +65,27 @@ internal static string FormatTags(KeyValuePair[] labels) return sb.ToString(); } + // Escapes the '\', ',' and '=' characters in a tag key or value so the flattened + // "key=value,key=value" representation produced by FormatTags can be decoded without + // ambiguity. See the comment on FormatTags for the encoding details. + private static void AppendEscaped(StringBuilder sb, string? value) + { + if (string.IsNullOrEmpty(value)) + { + return; + } + + foreach (char c in value) + { + if (c is '\\' or ',' or '=') + { + sb.Append('\\'); + } + + sb.Append(c); + } + } + internal static string FormatObjectHash(object? obj) => obj is null ? string.Empty : RuntimeHelpers.GetHashCode(obj).ToString(CultureInfo.InvariantCulture); } diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs index a5cdcb7f9b5626..1fb649b291e780 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs @@ -125,7 +125,8 @@ public void CollectionStop(string sessionId, DateTime intervalStartTime, DateTim WriteEvent(3, sessionId, intervalStartTime, intervalEndTime); } - [Event(4, Keywords = Keywords.TimeSeriesValues, Version = 2)] + // Version 3 escapes '\', ',' and '=' in the flattened 'tags' string (see Helpers.FormatTags). + [Event(4, Keywords = Keywords.TimeSeriesValues, Version = 3)] #if !NET [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", Justification = "This calls WriteEvent with all primitive arguments which is safe. Primitives are always serialized properly.")] @@ -135,7 +136,8 @@ public void CounterRateValuePublished(string sessionId, string meterName, string WriteEvent(4, sessionId, meterName, meterVersion ?? "", instrumentName, unit ?? "", tags, rate, value, instrumentId); } - [Event(5, Keywords = Keywords.TimeSeriesValues, Version = 2)] + // Version 3 escapes '\', ',' and '=' in the flattened 'tags' string (see Helpers.FormatTags). + [Event(5, Keywords = Keywords.TimeSeriesValues, Version = 3)] #if !NET [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", Justification = "This calls WriteEvent with all primitive arguments which is safe. Primitives are always serialized properly.")] @@ -145,7 +147,8 @@ public void GaugeValuePublished(string sessionId, string meterName, string? mete WriteEvent(5, sessionId, meterName, meterVersion ?? "", instrumentName, unit ?? "", tags, lastValue, instrumentId); } - [Event(6, Keywords = Keywords.TimeSeriesValues, Version = 2)] + // Version 3 escapes '\', ',' and '=' in the flattened 'tags' string (see Helpers.FormatTags). + [Event(6, Keywords = Keywords.TimeSeriesValues, Version = 3)] #if !NET [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", Justification = "This calls WriteEvent with all primitive arguments which is safe. Primitives are always serialized properly.")] @@ -158,7 +161,8 @@ public void HistogramValuePublished(string sessionId, string meterName, string? // Sent when we begin to monitor the value of a instrument, either because new session filter arguments changed subscriptions // or because an instrument matching the pre-existing filter has just been created. This event precedes all *MetricPublished events // for the same named instrument. - [Event(7, Keywords = Keywords.TimeSeriesValues, Version = 3)] + // Version 4 escapes '\', ',' and '=' in the flattened instrumentTags/meterTags strings (see Helpers.FormatTags). + [Event(7, Keywords = Keywords.TimeSeriesValues, Version = 4)] #if !NET [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", Justification = "This calls WriteEvent with all primitive arguments which is safe. Primitives are always serialized properly.")] @@ -183,7 +187,8 @@ public void BeginInstrumentReporting( // Sent when we stop monitoring the value of a instrument, either because new session filter arguments changed subscriptions // or because the Meter has been disposed. - [Event(8, Keywords = Keywords.TimeSeriesValues, Version = 3)] + // Version 4 escapes '\', ',' and '=' in the flattened instrumentTags/meterTags strings (see Helpers.FormatTags). + [Event(8, Keywords = Keywords.TimeSeriesValues, Version = 4)] #if !NET [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", Justification = "This calls WriteEvent with all primitive arguments which is safe. Primitives are always serialized properly.")] @@ -218,7 +223,8 @@ public void InitialInstrumentEnumerationComplete(string sessionId) WriteEvent(10, sessionId); } - [Event(11, Keywords = Keywords.InstrumentPublishing, Version = 3)] + // Version 4 escapes '\', ',' and '=' in the flattened instrumentTags/meterTags strings (see Helpers.FormatTags). + [Event(11, Keywords = Keywords.InstrumentPublishing, Version = 4)] #if !NET [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", Justification = "This calls WriteEvent with all primitive arguments which is safe. Primitives are always serialized properly.")] @@ -265,7 +271,8 @@ public void MultipleSessionsNotSupportedError(string runningSessionId) WriteEvent(15, runningSessionId); } - [Event(16, Keywords = Keywords.TimeSeriesValues, Version = 2)] + // Version 3 escapes '\', ',' and '=' in the flattened 'tags' string (see Helpers.FormatTags). + [Event(16, Keywords = Keywords.TimeSeriesValues, Version = 3)] #if !NET [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", Justification = "This calls WriteEvent with all primitive arguments which is safe. Primitives are always serialized properly.")] @@ -297,7 +304,8 @@ public void Version(int Major, int Minor, int Patch) /// /// Used to send the value of a base 2 exponential histogram. /// - [Event(19, Keywords = Keywords.TimeSeriesValues, Version = 1)] + // Version 2 escapes '\', ',' and '=' in the flattened 'tags' string (see Helpers.FormatTags). + [Event(19, Keywords = Keywords.TimeSeriesValues, Version = 2)] #if !NET [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", Justification = "This calls WriteEvent with all primitive arguments which is safe. Primitives are always serialized properly.")] diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricOuterLoopTests/MetricEventSourceTests.cs b/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricOuterLoopTests/MetricEventSourceTests.cs index 84c578523f9aea..bf4061966293d9 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricOuterLoopTests/MetricEventSourceTests.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricOuterLoopTests/MetricEventSourceTests.cs @@ -36,6 +36,141 @@ public void GetInstanceMethodIsReflectable() Assert.True(o is EventSource, "Expected object returned from MetricsEventSource.GetInstance() to be assignable to EventSource"); } + // The flattened "key=value,key=value" tag string escapes '\', ',' and '=' in each key and + // value so the pairs can be decoded without ambiguity. See Helpers.FormatTags. + [Theory] + // No special characters => unchanged. + [InlineData("plain", "simple", "plain=simple")] + // Comma in the value must be escaped so it isn't mistaken for a pair separator. + [InlineData("comma", "a,b,c", "comma=a\\,b\\,c")] + // Equals sign in the value must be escaped so it isn't mistaken for the key/value separator. + [InlineData("equals", "x=1", "equals=x\\=1")] + // Backslash must be escaped so it isn't mistaken for an escape sequence on decode. + [InlineData("path", "C:\\temp", "path=C:\\\\temp")] + // Special characters in the key are escaped too. + [InlineData("a,b=c", "v", "a\\,b\\=c=v")] + // Realistic URL value combining commas and equals signs. + [InlineData("url", "/api/items?filter=red,blue&sort=name", "url=/api/items?filter\\=red\\,blue&sort\\=name")] + public void FormatTags_EscapesDelimiters(string key, string value, string expected) + { + KeyValuePair[] objectTags = new[] { new KeyValuePair(key, value) }; + Assert.Equal(expected, Helpers.FormatTags(objectTags)); + + KeyValuePair[] stringTags = new[] { new KeyValuePair(key, value) }; + Assert.Equal(expected, Helpers.FormatTags(stringTags)); + } + + [Fact] + public void FormatTags_MultiplePairs_AreCommaSeparated() + { + KeyValuePair[] tags = new[] + { + new KeyValuePair("comma", "a,b,c"), + new KeyValuePair("equals", "x=1"), + new KeyValuePair("plain", "simple"), + }; + + // Escaped delimiters within values, literal ',' between pairs and literal '=' between key and value. + Assert.Equal("comma=a\\,b\\,c,equals=x\\=1,plain=simple", Helpers.FormatTags(tags)); + } + + [Theory] + [InlineData("plain", "simple")] + [InlineData("comma", "a,b,c")] + [InlineData("equals", "x=1")] + [InlineData("path", "C:\\temp")] + [InlineData("mixed=key,name", "=leading,and,trailing=")] + [InlineData("url", "/api/items?filter=red,blue&sort=name")] + public void FormatTags_RoundTripsThroughReferenceDecoder(string key, string value) + { + // Validates that the escaping produced by FormatTags can be decoded back to the original + // key/value pairs without ambiguity. + string encoded = Helpers.FormatTags(new[] { new KeyValuePair(key, value) }); + + List> decoded = DecodeTags(encoded); + + KeyValuePair single = Assert.Single(decoded); + Assert.Equal(key, single.Key); + Assert.Equal(value, single.Value); + } + + // Reference decoder for the escaped "key=value,key=value" format produced by Helpers.FormatTags. + // '\' escapes the following character; an unescaped '=' separates the first key from its value; + // an unescaped ',' separates pairs. + private static List> DecodeTags(string encoded) + { + List> result = new(); + if (string.IsNullOrEmpty(encoded)) + { + return result; + } + + StringBuilder key = new(); + StringBuilder value = new(); + bool inValue = false; + for (int i = 0; i < encoded.Length; i++) + { + char c = encoded[i]; + if (c == '\\' && i + 1 < encoded.Length) + { + (inValue ? value : key).Append(encoded[++i]); + } + else if (c == '=' && !inValue) + { + inValue = true; + } + else if (c == ',') + { + result.Add(new KeyValuePair(key.ToString(), value.ToString())); + key.Clear(); + value.Clear(); + inValue = false; + } + else + { + (inValue ? value : key).Append(c); + } + } + + result.Add(new KeyValuePair(key.ToString(), value.ToString())); + return result; + } + + // End-to-end validation that tag keys/values containing the ',' pair separator, the '=' + // key/value separator, or the '\' escape character are escaped in the flattened tag string + // that MetricsEventSource publishes, so they can be decoded without ambiguity. + [Fact] + [OuterLoop("Slow and has lots of console spew")] + public async Task EventSourcePublishesTimeSeriesWithTagsContainingDelimiters() + { + using Meter meter = new Meter("TestMeterDelimiters"); + Counter c = meter.CreateCounter("counter1"); + + EventWrittenEventArgs[] events; + using (MetricsEventListener listener = new MetricsEventListener(_output, MetricsEventListener.TimeSeriesValues, IntervalSecs, "TestMeterDelimiters")) + { + await listener.WaitForCollectionStop(s_waitForEventTimeout, 1); + + c.Add(5, new KeyValuePair("url", "/api/items?filter=red,blue&sort=name")); + c.Add(6, new KeyValuePair("comma", "a,b,c"), new KeyValuePair("equals", "x=1")); + c.Add(7, new KeyValuePair("path", @"C:\temp")); + await listener.WaitForCollectionStop(s_waitForEventTimeout, 2); + + c.Add(12, new KeyValuePair("url", "/api/items?filter=red,blue&sort=name")); + c.Add(13, new KeyValuePair("comma", "a,b,c"), new KeyValuePair("equals", "x=1")); + c.Add(14, new KeyValuePair("path", @"C:\temp")); + await listener.WaitForCollectionStop(s_waitForEventTimeout, 3); + events = listener.Events.ToArray(); + } + + // Expected escaped tag strings: '\' => "\\", ',' => "\,", '=' => "\="; the ',' between + // pairs and the '=' between a key and its value remain literal delimiters. + AssertCounterEventsPresent(events, meter.Name, c.Name, @"url=/api/items?filter\=red\,blue&sort\=name", "", ("5", "5"), ("12", "17")); + AssertCounterEventsPresent(events, meter.Name, c.Name, @"comma=a\,b\,c,equals=x\=1", "", ("6", "6"), ("13", "19")); + AssertCounterEventsPresent(events, meter.Name, c.Name, @"path=C:\\temp", "", ("7", "7"), ("14", "21")); + AssertCollectStartStopEventsPresent(events, IntervalSecs, 3); + } + // Tests that version event from MetricsEventSource is fired. [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] public void TestVersion()