Perf log #1
Perf log #1
Conversation
| } | ||
|
|
||
| [DllImport("kernel32.dll")] | ||
| internal static extern bool GlobalMemoryStatusEx(ref MEMORYSTATUSEX lpBuffer); |
There was a problem hiding this comment.
src/Cli/dotnet/Program.cs
Outdated
| if (!parseResult.Errors.Any()) | ||
| { | ||
| PerformanceLogEventSource.Log.TelemetrySendIfEnabledStart(); |
There was a problem hiding this comment.
this is a logging of a logging. Also TelemetryEventEntry.SendFiltered(parseResult) will raise an event then exit
There was a problem hiding this comment.
My goal here is to make sure that sending the Telemetry event doesn't take any appreciable period of time. Are you saying that this method just saves the event in-process and then returns, and that the background thread will pick it up and actually send it?
In reply to: 505726958 [](ancestors = 505726958)
There was a problem hiding this comment.
Gotcha. So ideally, this really turns into something that is never large. The cost of emitting these events is low, and so I'm inclined to include them for completeness - I'd like to be able to say that each action that happens on the main thread is covered by a set of events. Any concerns with this?
There was a problem hiding this comment.
It would be a problem on the consumer side (who run the query and do the analysis). I don't know a good way to document what the event is actually measuring especially when someone has no context of the implementation detail of CLI. All they see is the event name. It happened few times from the existing log, and people make wrong business conclusion from it.
A workaround would be make the the event name very long so that it is not easily assumed. SendToSeparateWorkerThreadEventIfEnabledStart. It is ridiculous to look at, but it will also make people pause and think.
There was a problem hiding this comment.
Is your thought that people will look at a log and not know what some of the things mean, or turn these on when capturing a trace? If so, I try not to worry about about the naming too much. Generally, you have to have code context as well to understand how to interpret an event. That said, totally on-board to consider tweaking names to be clearer.
There was a problem hiding this comment.
We also made it easy for the customer to read these logs. People could easily get "sending logs are super fast" conclusion by looking at the result, in fact it is at least 100ms due to web API call. But if it is opt-in, and we don't advertise people to read it, that should be fine
| { | ||
| // TODO: Should we choose something that is guaranteed to be unique? | ||
| _processIDStr = Process.GetCurrentProcess().Id.ToString(); | ||
| string logFilePath = Path.Combine(logDirectory, $"perf-{_processIDStr}.log"); |
There was a problem hiding this comment.
We have https://github.com/dotnet/sdk/blob/b1223209644d900702287faea8e9b71f95ec49f8/src/Cli/dotnet/Telemetry/PersistenceChannel/StorageService.cs (copied from Hockeyapp SDK) pretty much doing the samething. We should try to reuse it
There was a problem hiding this comment.
The file you referenced does look similar. Considering using it does seem reasonable, but we would also need to make sure that the additional functionality required by the perf log is there (e.g. treating individual logs as directories of files rather than individual files as telemetry does).
In reply to: 505751742 [](ancestors = 505751742)
|
Also could you point me to which part ETW get used? Or there is no ETW. |
|
There's no code here that does anything specific with ETW. All of the events that are emitted via PerformanceLogEventSource can also be consumed via ETW. EventSource provides this functionality OOB. In reply to: 709518380 [](ancestors = 709518380) |
|
Adding @davidfowl and @DamianEdwards so that they can take a look as well. @wli3 is helping review this code so that we can productize it. |
If I want to add ETW, I need to write specific code in |
No, I mis-understood what you were asking. Apologies. EventSource handles writing to ETW. You just have to turn on the provider using an ETW-based collector, such as PerfView. |
|
@brianrob and you can just send a PR to release/5.0.2xx after you addressed the comments |
No description provided.