Skip to content
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

Add RequestStackwalk parameter to EventPipeSession #4290

Merged
merged 3 commits into from
Nov 21, 2023

Conversation

ezsilmar
Copy link
Contributor

@ezsilmar ezsilmar commented Oct 3, 2023

Client-side of dotnet/runtime#84077 and the implementation of #3696.

To simplify the interface I made EventPipeSessionConfiguration public and introduced a new method in the DiagnosticsClient: Task<EventPipeSession> StartEventPipeSessionAsync(EventPipeSessionConfiguration configuration, CancellationToken token). This is the only method that supports disabling the stackwalk so no additional overloads with a new bool parameter and no synchronous counterpart. I believe it'd be easier to use and maintain a single async method with the options rather than creating more overloads or default parameters but I may not have all the context here so please correct me if you think it's a bad idea.

To deal with the backward compatibility I only use CollectTracingV3 when necessary i.e. when RequestStackwalk option is set to false. I think it's a good compromise between the added complexity and potentially surprising behavior:

  • when the client is old and the runtime is new everything works because the runtime supports CollectTracingV2
  • when the client is new but the runtime is old everything works until the new option is used. When it's used the session won't start as CollectTracingV3 doesn't exist server side: there'd be no clear error message but it's documented in the option summary.
  • when both the client and the runtime are new either CollectTracingV2 or CollectTracingV3 may be used transparently for the user
  • we may use the same trick when we introduce CollectTracingV4

The alternative is to implement version negotiation of some sort but I'd like to have your opinion before attempting this as handling the errors correctly wouldn't be easy (f.e. in my current fork I just hide the exception)

The testing turned out to be a bit complex as I needed to convert EventPipe stream to TraceLog to be able to read the stacktraces. I couldn't achieve that without writing data to a file. Afaiu the stackwalk may not work correctly without the rundown that only happens at the end of the session so I wonder if looking at the stacktraces with a live session is even possible (though iirc netfw+ETW could do that back in the days) ?

Thanks for your time !

@ezsilmar ezsilmar requested a review from a team as a code owner October 3, 2023 16:56
@noahfalk
Copy link
Member

noahfalk commented Oct 6, 2023

Hey @ezsilmar sorry for the delay. @davmason is probably the best person to go through this but he is off work this week. I believe he'll be back on Tuesday.

@ezsilmar
Copy link
Contributor Author

ezsilmar commented Oct 6, 2023

Hi @noahfalk, np, thanks for your reply! Anyway this feature is in progress for half a year already :)

@ezsilmar
Copy link
Contributor Author

Hi @davmason @noahfalk could you please have a look at this PR?

Copy link
Member

@davmason davmason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, sorry about missing this while I was out

@davmason davmason merged commit 97de881 into dotnet:main Nov 21, 2023
19 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants