Escape delimiters in metric event tags - #132262
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 30661229-7799-460e-b594-c6f27c79e3fe
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @steveisok, @dotnet/area-system-diagnostics-tracing |
There was a problem hiding this comment.
Pull request overview
Updates System.Diagnostics.DiagnosticSource metric event payloads so the flattened tags strings (key=value,key=value) are unambiguous when tag keys/values contain delimiter characters. This is done by escaping \, ,, and = in keys/values, and by bumping event versions for tag-carrying events so version-aware consumers can distinguish the new encoding.
Changes:
- Escape
\,,, and=inHelpers.FormatTagsfor bothIEnumerable<KeyValuePair<string, object?>>andKeyValuePair<string, string>[]inputs. - Increment
MetricsEventSourceevent versions for all events that emit flattened tag strings (including instrument/meter tags). - Add focused unit coverage for the encoding + a targeted end-to-end EventSource test validating the emitted escaped payload.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Helpers.cs | Implements delimiter escaping in FormatTags via a shared AppendEscaped helper. |
| src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs | Bumps event versions for tag-emitting events to distinguish escaped representation. |
| src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricOuterLoopTests/MetricEventSourceTests.cs | Adds unit + end-to-end tests validating escaping and emitted EventSource payload tags. |
noahfalk
left a comment
There was a problem hiding this comment.
Please update the PR description to link to the bug being fixed. I assume it is supposed to reference dotnet/diagnostics#5935?
heads up @tarekgh
| // 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: | ||
| // '\' => "\\" ',' => "\," '=' => "\=" |
There was a problem hiding this comment.
Escaping '\' as "\\" would be a breaking change on something that I assume currently works? I'd prefer we pick an escaping strategy that doesn't introduce new failures on existing tools. For example:
',' -> ",,"
'=' -> "=="
Admitedly its a little weird but I don't imagine there are going to be that many parsers for this event data in the world.
There was a problem hiding this comment.
agreed that since this scheme is not enough and this is close to platform complete - we'll park this until .NET 12.
Fixes dotnet/diagnostics#5935
Escapes backslashes, commas, and equals signs in metric tag keys and values emitted through
MetricsEventSource. Event versions are incremented so version-aware consumers can distinguish the escaped representation, and coverage validates the encoding and emitted payload.Compatibility: Existing tools continue to work for tags without these characters. Older tools do not understand the escaped representation from a newer runtime: commas can be treated as tag separators and may break legacy
dotnet-countersrendering, equals signs can be truncated or misparsed, and backslashes can appear doubled. Updated tooling should use the event version to decode tags.Note
This pull request description was generated by GitHub Copilot.