Fix dashboard log parsing in host #5425
Conversation
JamesNK
added
the
area-app-model
|
|
||
| namespace Aspire.Dashboard.ConsoleLogs; | ||
|
|
||
| internal static partial class TimestampParser |
There was a problem hiding this comment.
Note that this is mostly a copy and paste from what is in the dashboard. There will be duplicate copies after this PR but this will be cleaned up by #5398. I did this to minimize changes.
|
/backport to release/8.2 |
|
Started backporting to release/8.2: https://github.com/dotnet/aspire/actions/runs/10545263397 |
|
Do we need to handle 8.2 apphost with 8.1 dashboard? |
Not really, unless someone is really going out of their way to force a downgrade of the dashboard, but beginning with 8.2, the dashboard version will always match the reference to the apphost package. |
|
The timestamps in dashboard logs are added by DCP. And DCP adding the timestamp is configured by the apphost. And reading the logs is in the apphost. all the changes are in the apphost so there is no concern about mixing versions of apphost, DCP or dashboard. |
|
|
||
| internal static partial class TimestampParser | ||
| { | ||
| private static readonly Regex s_rfc3339RegEx = GenerateRfc3339RegEx(); |
There was a problem hiding this comment.
The generated implementation of
GenerateRfc3339RegEx()similarly caches a singletonRegexinstance, so no additional caching is needed to consume code.
There was a problem hiding this comment.
I want to minimize changes here and for 8.2 backport. I'll make regex changes in #5398
| if (match.Success) | ||
| { | ||
| var span = text.AsSpan(); | ||
| var timestamp = span[match.Index..(match.Index + match.Length)]; |
There was a problem hiding this comment.
I think this can just be:
| var timestamp = span[match.Index..(match.Index + match.Length)]; | |
| var timestamp = match.ValueSpan; |
| // Z - Literal, same as +00:00 | ||
| // (?:[Z+-](?:[01][0-9]|2[0-3]):(?:[0-5][0-9])) - Time Zone offset, in the form ZHH:MM or +HH:MM or -HH:MM | ||
| // | ||
| // Note: (?:) is a non-capturing group, since we don't care about the values, we are just interested in whether or not there is a match |
There was a problem hiding this comment.
Using RegexOptions.ExplicitCapture instead of ?: simplifies the pattern, and ensures that no groups are accidentally captured.
Consider also adding RegexOptions.CultureInvariant.
| // Explanation: | ||
| // ^ - Starts the string | ||
| // (?:\\d{4}) - Four digits for the year | ||
| // - - Separator for the date | ||
| // (?:0[1-9]|1[0-2]) - Two digits for the month, restricted to 01-12 | ||
| // - - Separator for the date | ||
| // (?:0[1-9]|[12][0-9]|3[01]) - Two digits for the day, restricted to 01-31 | ||
| // [T ] - Literal, separator between date and time, either a T or a space | ||
| // (?:[01][0-9]|2[0-3]) - Two digits for the hour, restricted to 00-23 | ||
| // : - Separator for the time | ||
| // (?:[0-5][0-9]) - Two digits for the minutes, restricted to 00-59 | ||
| // : - Separator for the time | ||
| // (?:[0-5][0-9]) - Two digits for the seconds, restricted to 00-59 | ||
| // (?:\\.\\d{1,9}) - A period and up to nine digits for the partial seconds | ||
| // Z - Literal, same as +00:00 | ||
| // (?:[Z+-](?:[01][0-9]|2[0-3]):(?:[0-5][0-9])) - Time Zone offset, in the form ZHH:MM or +HH:MM or -HH:MM | ||
| // | ||
| // Note: (?:) is a non-capturing group, since we don't care about the values, we are just interested in whether or not there is a match | ||
| [GeneratedRegex("^(?:\\d{4})-(?:0[1-9]|1[0-2])-(?:0[1-9]|[12][0-9]|3[01])T(?:[01][0-9]|2[0-3]):(?:[0-5][0-9]):(?:[0-5][0-9])(?:\\.\\d{1,9})?(?:Z|(?:[Z+-](?:[01][0-9]|2[0-3]):(?:[0-5][0-9])))?")] |
There was a problem hiding this comment.
The duplication here can be avoided by using RegexOptions.IgnorePatternWhitespace and including inline comments in the regex pattern. This means they can never get out of sync.
Something like:
[GeneratedRegex("""
^ # Starts the string
\d{4} # Four digits for the year
- # Separator for the date
(0[1-9]|1[0-2]) # Two digits for the month, restricted to 01-12
- # Separator for the date
(0[1-9]|[12][0-9]|3[01]) # Two digits for the day, restricted to 01-31
[T ] # Literal, separator between date and time, either a T or a space
([01][0-9]|2[0-3]) # Two digits for the hour, restricted to 00-23
: # Separator for the time
([0-5][0-9]) # Two digits for the minutes, restricted to 00-59
: # Separator for the time
[0-5][0-9] # Two digits for the seconds, restricted to 00-59
\.\d{1,9} # A period and up to nine digits for the partial seconds
Z # Literal, same as +00:00
[Z+-]([01][0-9]|2[0-3]):[0-5][0-9] # Time Zone offset, in the form ZHH:MM or +HH:MM or -HH:MM
""",
RegexOptions.ExplicitCapture | RegexOptions.InvariantCulture | RegexOptions.IgnorePatternWhitespace)]
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Fixes regression from #5357
There is code in the Aspire host that listens to dashboard logs. It expects JSON logs from the dashboard, which are parsed and possibly written to the host. That allows for dashboard errors to be visible in the host console.
The previous PR changes Aspire to add a timestamp to executable log content, including the dashboard. Parsing timestamp prefixed messages as JSON fails.
Unfortunatly there were no unit tests here and there is no indication when running the host that there are errors when parsing the logs.
Customer Impact
Customer won't see errors from the dashboard in the Aspire host console.
Testing
Unit tests and manual testing.
Before (no error):
After (error in console):
Risk
Low. Product change is to trim a timestamp (if present) from content before deserializing it.
Regression?
Yes
Checklist
<remarks />and<code />elements on your triple slash comments?Microsoft Reviewers: Open in CodeFlow