-
Notifications
You must be signed in to change notification settings - Fork 49
Fix tracing pass-through structure #1052
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
Conversation
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.
Pull Request Overview
This PR fixes the structure of pass-through traces from child processes by changing how process ID (pid), target, and line number are formatted. Instead of prefixing the PID to the message string, these fields are now structured as separate fields in the trace log.
Key changes:
- Refactored trace logging to use structured fields instead of concatenated strings
- Updated test coverage to verify the new structured format includes pid, target, and line_number fields
- Improved test robustness with better error reporting and validation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
dsc_lib/src/dscresources/command_resource.rs | Refactored log_stderr_line function to use structured logging fields instead of string formatting |
dsc/tests/dsc_tracing.tests.ps1 | Enhanced test to validate structured fields (pid, target, line_number) and improved error reporting |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Rebased #1048 on this PR for local testing, and I have some findings with example output after:
Updated output examples
|
@michaeltlombardi per tokio-rs/tracing#2419 those top level properties can't be overwritten at this time |
With it being a limitation of the library, I think we can defer the problem of bubbling up the I'm not sure how we can bubble up the additional fields at this time - right now I think we're just losing We may also want to provide some namespacing for the nested fields to more clearly differentiate them from the top-level fields - it can be confusing to see two different values for {
"timestamp": "2025-08-13T22:17:34.44459Z",
"level": "WARN",
"fields": {
"message": "structured trace message for StructWithMetadataAndAdditionalFields",
"process.pid": 23248,
"process.target": "C:\\code\\dsc\\docs\\bin\\debug\\repro.trace.resource.ps1",
"process.line_number": 94,
"process.timestamp": "2024-08-13T22:17:34.24359Z"
},
"target": "dsc_lib::dscresources::command_resource",
"line_number": 870
} Regarding the missing data, maybe we can just collect it into a map, like: {
"timestamp": "2025-08-13T22:17:34.44459Z",
"level": "WARN",
"fields": {
"message": "structured trace message for StructWithMetadataAndAdditionalFields",
"process.pid": 23248,
"process.target": "C:\\code\\dsc\\docs\\bin\\debug\\repro.trace.resource.ps1",
"process.line_number": 94,
"process.timestamp": "2024-08-13T22:17:34.24359Z",
"process.data": {
"extraInteger": 10,
"extraString": "additional data"
},
"target": "dsc_lib::dscresources::command_resource",
"line_number": 870
} |
I spent some time trying to see how we could get There are two open (for a while) PRs for handling this:
Either of them being merged would more or less resolve this problem for us nicely, but regardless we would have to add the I'm now thinking we may want to defer structured message data until we implement OpenTelemetry, where we can re-examine the model for bubbling up messages from invoked commands. |
PR Summary
This fixes how structured pass-through traces from child processes are correctly structured by DSC. The process id (pid) is now a field instead of being prefixed to the message, this is also true for target and line number if specified.
PR Context
Address part of the issues raised in #1048