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

Optionally record activities in the service #13835

Merged
merged 58 commits into from
Nov 21, 2022
Merged

Conversation

safesparrow
Copy link
Contributor

@safesparrow safesparrow commented Sep 3, 2022

Follow up on @baronfel 's initial work.

This PR adds activity recording to the FCS service.

It adds a dependency on System.Diagnostics.DiagnosticSource but nothing else.

Recording and exporting of activities can be enabled in user code (eg. in the IDE or external tests) - example.

Sample traces

Sample traces produced can be found in https://github.com/safesparrow/fsharp-benchmark-generator/tree/main/trace_samples
They can be opened with Jaeger UI.

Jaeger UI screenshots:

Fantomas sample (codebase) image
stress_huge_dense sample (codebase) image

Sample traces for perf analysis of #13521

The driver for this PR is the need to easily verify the behaviour of the compiler/service, especially w.r.t. multi-threaded work.

With that in mind, I produced the following traces showcasing the behaviour of parallel project analysis:

50_leaves sample, parallel analysis on, server GC image
50_leaves sample, parallel analysis on, workstation GC image
50_leaves sample, parallel analysis off, server GC image
50_leaves sample, parallel analysis off, workstation GC image

Generating traces

Sample traces can be generated with the following tools:

# Run Jaeger docker image - command from https://www.jaegertracing.io/docs/1.37/getting-started/
docker run -d --name jaeger -e COLLECTOR_ZIPKIN_HOST_PORT=:9411 -e COLLECTOR_OTLP_ENABLED=true   -p 6831:6831/udp   -p 6832:6832/udp   -p 5778:5778   -p 16686:16686   -p 4317:4317   -p 4318:4318   -p 14250:14250   -p 14268:14268   -p 14269:14269   -p 9411:9411   jaegertracing/all-in-one:1.37

# Install 'fcs-benchmark' global dotnet tool - https://github.com/safesparrow/fsharp-benchmark-generator
dotnet tool install fcsbenchmark --global --prerelease

# Run a benchmark of a sample solution, using local F# codebase, and 4 different GC&threading configurations
fcs-benchmark --sample 50_leaves --local=C:/projekty/fsharp/fsharp-otel -c C:/.artifacts --gc=Compare --parallel-analysis=Compare --record-otel-jaeger -w 0 -n 1

Notes

Names of the activities and tags are likely wrong - please do suggest better names🙂

To do:

Copy link
Member

@baronfel baronfel left a comment

Choose a reason for hiding this comment

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

Nice - I think it might demonstrate the value better if you could attach trace views to show the results.

NuGet.config Outdated Show resolved Hide resolved
src/Compiler/Service/IncrementalBuild.fs Outdated Show resolved Hide resolved
src/Compiler/Service/service.fs Outdated Show resolved Hide resolved
src/fsc/fsc.targets Outdated Show resolved Hide resolved
NuGet.config Outdated Show resolved Hide resolved
@safesparrow
Copy link
Contributor Author

Have attached Jaeger screenshots now and removed any dependencies except for System.Diagnostics.DiagnosticSource.

@T-Gro T-Gro requested review from vzarytovskii and 0101 October 24, 2022 09:57
@vzarytovskii
Copy link
Member

We can merge after @KevinRansom looks at it

@safesparrow safesparrow requested a review from a team as a code owner October 26, 2022 00:11
Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Thanks for this, it looks good. However, we will need to do some due diligence internally to ensure that this satisfies our telemetry and privacy policies. We will add this to our November milestone to see if we can get it approved.

Planning issue here: #14210

@safesparrow
Copy link
Contributor Author

Thanks for this, it looks good. However, we will need to do some due diligence internally to ensure that this satisfies our telemetry and privacy policies. We will add this to our November milestone to see if we can get it approved.

Planning issue here: #14210

Thanks for looking.

OOI what makes this more sensitive than any other existing logging? Also it doesn't depend on open telemetry and is off by default. Is there some increased risk of user data leakage or something like that?

@safesparrow
Copy link
Contributor Author

safesparrow commented Nov 20, 2022

@vzarytovskii If this gets merged, how difficult will it be to add new activities for other areas of the compiler in future PRs? I'm looking into optimization and having some detailed tracing there would help (eg. a span for each file being optimized, similar to how parsing & type-checking work atm).

Will it require new approvals from the policy PoV?

@vzarytovskii
Copy link
Member

@vzarytovskii Vlad Zarytovskii FTE If this gets merged, how difficult will it be to add new activities for other areas of the compiler in future PRs? I'm looking into optimization and having some detailed tracing there would help (eg. a span for each file being optimized, similar to how parsing & type-checking work atm).

Will it require new approvals from the policy PoV?

Yep, it got approved in principle. Since compiler doesn't work with PII or EUII per se, we can just add new metrics, as long as we don't opt-in them by default (and we don't for neither old or new telemetry).

@vzarytovskii
Copy link
Member

@safesparrow It's ready to merge, let me know if you want any more changes here, or if we can merge it now.

@T-Gro
Copy link
Member

T-Gro commented Nov 21, 2022

That is great news, let's merge it in so that new ones can build on this module.

@vzarytovskii vzarytovskii merged commit f0143ef into dotnet:main Nov 21, 2022
@safesparrow
Copy link
Contributor Author

safesparrow commented Nov 21, 2022

I'm happy to have it merged, and we can iterate easily in future. Thanks for help all 👍

@vzarytovskii
Copy link
Member

This needs to be reverted: #14413

@vzarytovskii
Copy link
Member

Revert has to also go directly to 17.5

@vzarytovskii
Copy link
Member

Or, as Kevin pointed out, that might be solved by adding package to vsix.

TheAngryByrd added a commit to TheAngryByrd/FsAutoComplete that referenced this pull request Jul 9, 2023
TheAngryByrd added a commit to TheAngryByrd/FsAutoComplete that referenced this pull request Jul 9, 2023
TheAngryByrd added a commit to ionide/FsAutoComplete that referenced this pull request Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants