Ability to request stackwalk per EventPipe session

[ezsilmar]

Server-side part of dotnet/diagnostics#3696

I'm not sure about the interaction between DOTNET_EventPipeEnableStackwalk and the flag in the IPC. Currently the env variable overwrites the behavior requested by the IPC, so if the stackwalk is disabled application-wide it can't be enabled from the outside.

Also I could not build the change locally due to dotnet/msbuild#8532 even though it was merged 3 weeks ago (I'm on the tip of the main branch in dotnet/runtime and executed git clean -xfd). What should I do to get the msbuild changes?

[ezsilmar]

There are couple of places that need further work, for instance mono/wasm integration and ProfToEEInterface. I'll add them in the coming days.

I'd appreciate some general feedback in the meantime and any help with making the build work locally. Thanks!

[tommcdon]

@ezsilmar thanks for submitting this PR! Were you able to workaround the local build issues? If yes, is the PR ready for review or would it be OK to change it to "draft" while more work is being done?

It looks like this PR was submitted to collect some early feedback. @lateralusX do you have any feedback to share for the changes in this pull request?

[lateralusX]

@ezsilmar thanks for the contribution! Overall I think it looks reasonable and inline with how we extended the collect command with additional parameters in the past. What could be argued is when we add a new collect command, maybe its time to use a key/value pair for session properties so we can extend without adding new commands going forward. The new collect3 command would then pretty much carry session parameters array (key/value pairs) and provider config array and we encode all the current values into key/value pairs, CircleBufferSize, SerializationFormat, RundownRequested, StacksRequested. Parsing of this should be straight forward, as well as error handling when getting unrecognized parameters, values etc. Parameters should be optional and backed by reasonable default values. @noahfalk any thoughts on that? Since we add a new command I think we should consider simplifying the extendibility, since these changes needs to bubble all the way up to the tools to be useful. Tools like dotnet-trace could then pass additional properties using known key/value pairs that make sense to change for a session, like this new stackwalk option or what ever option we might come up with in the future that is session specific.

[ezsilmar]

@lateralusX, @tommcdon, thanks for your comments!

I think it's indeed worth it to pass session properties as key/value pairs, that would also make the client API cleaner.

I'm busy with something else at work at the moment, but I should be able to come back to this PR next week. I don't know if the build issues were resolved, I'll ping here as soon as I try. Feel free to move the PR to draft if you need.

[ezsilmar]

Hi, just wanted to say my local build issue is resolved. I then ran into #34649 but /p:NativeOptimizationDataSupported=false works so it's good enough.

Regarding the PR, I'll try to do the following:

• remove the changes from ep_enable and ep_enable_2 to limit the "bubbling up" of the feature
• create ep_enable_3 (ep_session_options*) where the options would have sane defaults and try to limit the usage of ep_enable_3 to the IPC for now
• rename the parameter to disable_stacktrace to make its interaction with the env variable less confusing (the env variable takes precedence)
• as IPC extensibility sparks the discussion I prefer to track in a separate issue and follow the current pattern for now
• share the code of ds_eventpipe_collect_tracing_command_payload_free and alike between the command versions

Also I'm going to be on vacation next week. We should probably put the change in draft until I make it compile.

Thanks again for your comments!

[tommcdon]

@lateralusX @noahfalk @davmason please take a look

[tommcdon]

@lateralusX @noahfalk @davmason please take a look

@lateralusX @noahfalk @davmason - friendly ping

[lateralusX]

The approach described in #84077 (comment) seems to be inline with discussions in this PR and straightforward to implement. Next step would be to adjust this PR along those lines and we can do another round of reviews once that is done.

[tommcdon]

@ezsilmar do you plan to make the suggested code changes to the PR? Otherwise would you mind changing it to 'draft' mode?

[ezsilmar]

@tommcdon I plan to work on it in July. I put it to draft for now and I'll re-open when it's ready for review.

[ghost]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[davmason]

I took a look at the PR, it looks good to me. I don't have anything to add that Noah or Johan haven't already pointed out.

[tommcdon]

@noahfalk @lateralusX there has been a lot of discussion and feedback on this PR. Is everything addressed or do we have a crisp list of outstanding work needed to merge it?

[ezsilmar]

@tommcdon there were some minor comments left. I plan to address them next week when I’m back from vacation. This change indeed took much more time than I anticipated but I think it’s really close to be merged :)

[noahfalk]

@noahfalk @lateralusX there has been a lot of discussion and feedback on this PR. Is everything addressed or do we have a crisp list of outstanding work needed to merge it?

As far as I know everything was addressed, except I think we still need testing and docs: https://github.com/dotnet/runtime/pull/84077/files#r1303870907

[lateralusX]

Once the last comments have been addressed and CI is green this is ready to go.

[tommcdon]

@tommcdon there were some minor comments left. I plan to address them next week when I’m back from vacation. This change indeed took much more time than I anticipated but I think it’s really close to be merged :)

Hi @ezsilmar - thanks for the diagnostics repo PR! Are there any other work items left or is this PR ready?

[ezsilmar]

Hi @tommcdon, it’s ready for review! There are two minor unresolved discussions above where I need your inputs but everything else was addressed.

The CI is failing but I think it’s unrelated, we should try to re-run it. I’ll update the PR to the tip of the main branch on Monday if it doesn’t converge.

[tommcdon]

Hi @tommcdon, it’s ready for review! There are two minor unresolved discussions above where I need your inputs but everything else was addressed.

@lateralusX would you mind taking a look and providing additional feedback?

The CI is failing but I think it’s unrelated, we should try to re-run it. I’ll update the PR to the tip of the main branch on Monday if it doesn’t converge.

Sounds good!

[lateralusX]

LGTM! Thank you so much for this contribution!

[ezsilmar]

@lateralusX it didn't merge after your approval, should we ping other team members? Thanks!

[tommcdon]

@lateralusX it didn't merge after your approval, should we ping other team members? Thanks!

Pinging @noahfalk @davmason for their acknowledgement

[noahfalk]

Acknowledging by merging 👍
Congrats its in!