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

profiling streaming TraceLog #2385

Merged
merged 42 commits into from
Aug 23, 2023
Merged

profiling streaming TraceLog #2385

merged 42 commits into from
Aug 23, 2023

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented May 22, 2023

Addresses multiple issues with our current TraceLog integration:

  1. originally, we processed profiles by writing them to a file and then loading that, processing events and removing - this PR ensures all events are processed as they happen
  2. originally, we started a new DiagnositcsClient profiler session on each transaction - this is very slow (100+ms) so this PR changes the logic to have a single profiler session running all the time and attach/detach our processing logic for each transaction

Additionally, this PR fixes some bugs in the profiler & adds it as a submodule, with a ProjectReference dependency on it. Currently, the submodule points to microsoft/perfview#1867

#skip-changelog because this isn't public-facing

@mattjohnsonpint
Copy link
Contributor

Looking forward to getting this in. Thanks so much for your diligence! 🎉

@bruno-garcia
Copy link
Member

branched from this into this: #2555

@vaind vaind reopened this Aug 21, 2023
vaind added 2 commits August 21, 2023 21:47
commit 6a1df09
Author: Ivan Dlugos <[email protected]>
Date:   Mon Aug 21 14:19:53 2023 +0200

    timeout update

commit 833a319
Merge: eaa320b c1e4dad
Author: Ivan Dlugos <[email protected]>
Date:   Mon Aug 21 13:13:51 2023 +0200

    Merge branch 'main' into feat/profiling-low-overhead

commit eaa320b
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 20:30:09 2023 -0400

    debugging on sample

commit 1a593bc
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 20:10:34 2023 -0400

    fuck this slnf mess

commit b5cccee
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 17:53:03 2023 -0400

    add profile category

commit aa01035
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 16:35:52 2023 -0400

    pack profiling

commit 20286f4
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 16:35:18 2023 -0400

    new full sln file

commit 7c7404d
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 15:11:59 2023 -0400

    verify

commit f1ea1ec
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 14:34:44 2023 -0400

    fix tfm in script

commit 3a05994
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 14:16:24 2023 -0400

    fix benchmarks. Package it

commit bec4641
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 13:53:03 2023 -0400

    ignore obj

commit 791cdc6
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 13:52:01 2023 -0400

    fixed sln file with all but mobile

commit 915a0f2
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 13:29:28 2023 -0400

    sample

commit 67ef890
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 12:22:18 2023 -0400

    log on collection failure

commit 53715f4
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 11:46:30 2023 -0400

    set correct platform to profile

commit 43f78a0
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 11:20:07 2023 -0400

    gitignore on module

commit 40b6add
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 11:19:01 2023 -0400

    in SentryCore sln

commit 1c98746
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 11:14:02 2023 -0400

    builds!

commit 19e24a2
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 11:01:21 2023 -0400

    resolve errors

commit 6cb6cd1
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 10:20:55 2023 -0400

    fix structure

commit b346567
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 09:55:40 2023 -0400

    ignore build files from submodule perfview

commit 790d774
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 09:50:11 2023 -0400

    internal perfview

commit 6e2ae7c
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 09:41:09 2023 -0400

    remove profiling from sln/slnf

commit 3376bea
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 09:38:10 2023 -0400

    perfview under profiling

commit aee692b
Author: Bruno Garcia <[email protected]>
Date:   Sun Aug 20 08:54:41 2023 -0400

    perfview sampling

commit 0bbb0bd
Merge: 5ff8f1a b4f9485
Author: Bruno Garcia <[email protected]>
Date:   Sat Aug 19 18:56:53 2023 -0400

    Merge branch 'chore/integrate-traceevent' into feat/profiling-low-overhead

commit 5ff8f1a
Author: Bruno Garcia <[email protected]>
Date:   Sat Aug 19 18:54:43 2023 -0400

    profiling in core sln

commit b4f9485
Author: Ivan Dlugos <[email protected]>
Date:   Wed Aug 16 20:26:55 2023 +0200

    script & project updates

commit abc81db
Merge: 3a7a10f f457e3a
Author: Ivan Dlugos <[email protected]>
Date:   Wed Aug 16 19:23:29 2023 +0200

    Merge branch 'feat/streaming-tracelog' into chore/integrate-traceevent

commit 3a7a10f
Merge: 4ffb45a d915f98
Author: Stefan Jandl <[email protected]>
Date:   Wed Aug 16 16:28:53 2023 +0200

    Merge branch 'main' into chore/integrate-traceevent

commit 4ffb45a
Author: Bruno Garcia <[email protected]>
Date:   Mon Aug 14 15:08:31 2023 -0400

    submodule perfview sentry fork

commit 162ea2a
Author: Ivan Dlugos <[email protected]>
Date:   Sun Apr 23 23:04:20 2023 +0200

    prepare traceevent integration
@bruno-garcia bruno-garcia marked this pull request as ready for review August 22, 2023 17:23
@bruno-garcia bruno-garcia changed the title WIP: streaming TraceLog profiling streaming TraceLog Aug 22, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 22, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 614ae76

@@ -125,9 +139,9 @@ public void DiagnosticsSessionStartCopyStop(bool rundown, string provider)
{
EventPipeProvider[] providers = provider switch
{
"runtime" => new[] { new EventPipeProvider("Microsoft-Windows-DotNETRuntime", EventLevel.Informational, (long)ClrTraceEventParser.Keywords.Default) },
"runtime" => new[] { new EventPipeProvider("Microsoft-Windows-DotNETRuntime", EventLevel.Informational, (long)BenchmarkDotNetTransientTraceEvent::Microsoft.Diagnostics.Tracing.Parsers.ClrTraceEventParser.Keywords.Default) },
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to fix this in a follow up PR. Since our customers will experience the same thing. We need to change namespaces, or make stuff internal

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

related to #2385 (comment)

// | ThreadTransfer | GCHeapAndTypeNames | Codesymbols | Compilation,
new EventPipeProvider(ClrTraceEventParser.ProviderName, EventLevel.Informational, (long) ClrTraceEventParser.Keywords.Default),
new EventPipeProvider(SampleProfilerTraceEventParser.ProviderName, EventLevel.Informational),
// new EventPipeProvider(TplEtwProviderTraceEventParser.ProviderName, EventLevel.Informational, (long) TplEtwProviderTraceEventParser.Keywords.Default)
Copy link
Member

Choose a reason for hiding this comment

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

This isnt' needed now because there's no file involved?

Suggested change
// new EventPipeProvider(TplEtwProviderTraceEventParser.ProviderName, EventLevel.Informational, (long) TplEtwProviderTraceEventParser.Keywords.Default)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There were some experiments with this but it didn't give that much value in the past. However, the idea is that it would become necessary for profiling individual activities. As is, it cannot be used because the "activity computer" in perfview (that was used to handle these events) isn't yet updated to support streaming event pipe. It would have made the perfview PR too large to hope for a merge so I didn't do it yet. Hoping to get back to it once all this shakes down.

<PackageReference Include="Microsoft.Diagnostics.NETCore.Client" Version="0.2.421201" />
<PackageReference Include="Microsoft.Diagnostics.Tracing.TraceEvent" Version="3.1.2" />
<ProjectReference Include="../../modules/perfview/src/TraceEvent/TraceEvent.csproj" />
Copy link
Member

Choose a reason for hiding this comment

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

I imagine we'll need to figure out distributing this, also OK on a follow up PR since profilign isn't packaged yet

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, that was my thinking too

@bruno-garcia
Copy link
Member

Added 11f2b34 to deal with: microsoft/perfview#1900

If we need more packages from that feed, we need to opt-in by adding the specific name to the packageSource node

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

I'd say this is good to merge, and we have follow-ups. It's still not getting packed for NuGet so we are free to iterate on main

@vaind vaind force-pushed the feat/streaming-tracelog branch from 708d0bd to 7a00db9 Compare August 23, 2023 06:42
@vaind vaind merged commit 047acad into main Aug 23, 2023
@vaind vaind deleted the feat/streaming-tracelog branch August 23, 2023 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants