-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Log TaskParameterEvent for scalar parameters #9908
Conversation
Let's check that the diag output for console loggers doesn't change, as well as let's see how it looks like in the binlog for null, empty string, numbers, booleans and such. What is the current behavior for scalar parameters that have the default value? (0, null, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
+1 on quick manual test of binlog and FileLog appearance for the change
Done. No differences found with our textual logging (with task parameter logging enabled) or with the way it looks in the viewer. I have exported the tree as XML from the viewer and the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Just a note that the viewer can't remove parsing textual messages because we still need to read all legacy binlogs. |
Fixes #9827
Context
TaskParameterEvent
withTaskParameterMessageKind.TaskInput
is currently used only for parameters that are lists. Parameters that are simple strings are logged as a specially formatted low-importance message.The binlog viewer contains logic to recognize this special message and recover the Name and Value to be rendered in the viewer UI. Since we will use this event for analyzers, it would be unfortunate to add one more place with this suboptimal processing.
Changes Made
Unified the logic in
TaskExecutionHost
to log all parameters asTaskParameterEvent
withTaskParameterMessageKind.TaskInput
. The change is under a change wave check.Testing
SolutionConfigurationContents
parameter which is a string but the content looks formatted so it was rendered incorrectly as a list of items.Notes
As @KirillOsenkov pointed out in the issue, we don't really depend on the textual messages so we don't have to do any double-logging. The viewer can remove parsing the textual messages on its own schedule.