Improve VS Code and cross-platform dev experience #2997

tillig · 2022-03-09T01:02:57Z

Changes

Fixed changes that had reverted Improve Visual Studio Code support #2015: VS Code (OmniSharp) looks at the first framework listed in TargetFrameworks and uses that for things like code analysis and IntelliSense. Having them ordered from highest to lowest .NET Core, then the .NET desktop framework (e.g., net461) last helps.
Added .vscode folder with tasks for build and test as well as default settings and a couple extension recommendations to bootstrap folks. For example, C# dev tools aren't built into VS Code, they're an extension - so that's recommended.
Added global.json to help with tooling determining which version of the .NET SDK to use during build when dotnet is invoked.
Fixed one redundant cast analysis warning which was the only warning that was really throwing OmniSharp for a loop, at least on Mac. It was saying "System.Enum is not defined" but the underlying problem was the redundant cast.

There are definitely still some cross-platform challenges in VS Code. For example, while the Microsoft.NETFramework.ReferenceAssemblies package allows you to compile, OmniSharp analysis really just doesn't work so there are ~549 errors that all are basically The type 'XXXXX' is defined in an assembly that is not referenced. You must add a reference to assembly 'netstandard, Version=2.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. [OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule] Build works great, OmniSharp, not so much.

Regardless, this will help folks with VS Code get up and running a little easier, I think.

PR open-telemetry#2015 fixed the target framework ordering to allow better VS Code due to the challenges with VS Code multi-targeting: dotnet/vscode-csharp#1783 dotnet/vscode-csharp#3754 Short version: VS Code support is better if you list the latest .NET Core frameworks first. Recent projects and updates have lost that ordering so VS Code isn't great on non-Windows. This restores that order and updates the latest projects.

The .vscode folder isn't per-user like .user files or .vs folders are in Visual Studio. Instead, this is where the build tasks, etc. are kept to allow VS Code to build and test from inside the IDE.

Includes a small number of recommended but common extensions to make .NET development easier. Settings support those extensions. Default build and test tasks are included.

If it's in parallel then test discovery is also in parallel, which can really tank your CPU at IDE startup.

This is the only analysis warning that really throws OmniSharp for a loop on Mac. It can't figure out if System.Enum is in a missing assembly reference or what; this fixes it.

global.json

test/OpenTelemetry.Tests.Stress/OpenTelemetry.Tests.Stress.csproj

codecov · 2022-03-09T02:05:52Z

Codecov Report

Merging #2997 (bf24bb1) into main (af6a5d2) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2997      +/-   ##
==========================================
+ Coverage   84.08%   84.15%   +0.06%     
==========================================
  Files         258      258              
  Lines        9093     9093              
==========================================
+ Hits         7646     7652       +6     
+ Misses       1447     1441       -6

Impacted Files	Coverage Δ
src/OpenTelemetry.Api/Internal/StatusHelper.cs	`100.00% <100.00%> (ø)`
...ZPages/Implementation/ZPagesExporterEventSource.cs	`56.25% <0.00%> (-6.25%)`	⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	`76.47% <0.00%> (-5.89%)`	⬇️
...mentation/ExportClient/BaseOtlpGrpcExportClient.cs	`62.50% <0.00%> (+12.50%)`	⬆️
...entation/ExportClient/OtlpGrpcTraceExportClient.cs	`50.00% <0.00%> (+14.28%)`	⬆️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs	`59.09% <0.00%> (+22.72%)`	⬆️

cijothomas

This looks reasonable, left a minor comment about this.
Would be great if we get another approver who is a VSCode user to take a look. @pellared ? @alanwest ?

reyang

LGTM. I have some concern regarding the lack of CI, but I think overall this is a good movement, it is okay to me if want to have the CI later (in a follow up PR, or wait until someone complains about it).

pellared

There are definitely still some cross-platform challenges in VS Code.

TBH I think you have done the best you could. Especially if one would like to develop on Linux or MacOs. Nice job 👍

In general, I do not see any blocking comments. However, I have concerns about the VS Code config-related changes (.vscode dir and global.json file). I would prefer to have it as a separate PR. It will be easier to roll back if one would find it harmful. Also currently the format GH workflow does not work b/c of the changes. Related comment: #2997 (comment)

.gitignore

.vscode/extensions.json

.vscode/settings.json

tillig · 2022-03-09T15:29:24Z

I'm really, really not interested in going back here to split out the .csproj and .vscode stuff into two separate PRs. If folks don't like the .vscode stuff, it's "delete the folder and add back to .gitignore" which is a pretty simple operation.

pellared · 2022-03-09T16:44:56Z

I'm really, really not interested in going back here to split out the .csproj and .vscode stuff into two separate PRs. If folks don't like the .vscode stuff, it's "delete the folder and add back to .gitignore" which is a pretty simple operation.

I was not aware that the maintainers changed their mind. Approving.

cijothomas · 2022-03-09T16:47:57Z

I'm really, really not interested in going back here to split out the .csproj and .vscode stuff into two separate PRs. If folks don't like the .vscode stuff, it's "delete the folder and add back to .gitignore" which is a pretty simple operation.

I was not aware that the maintainers changed their mind. Approving.

Thanks for reviewing.
We can revisit the decision, if the VSCode things becomes a headache to maintain.

alanwest

This looks reasonable, left a minor comment about this.
Would be great if we get another approver who is a VSCode user to take a look. @pellared ? @alanwest ?

Sorry I'm coming to this late... but

Holy crap. Hard approve. Just pulled latest and opened up VSCode. Things are way more seamless - especially when conditional compilation is involved.

I did have to make sure I had the latest .NET SDK installed, but that seems reasonable to me.

Thank you, @tillig!

tillig added 7 commits March 8, 2022 16:07

Stop ignoring .vscode

ac0e5db

The .vscode folder isn't per-user like .user files or .vs folders are in Visual Studio. Instead, this is where the build tasks, etc. are kept to allow VS Code to build and test from inside the IDE.

Basic build and test in VS Code.

28a941a

Includes a small number of recommended but common extensions to make .NET development easier. Settings support those extensions. Default build and test tasks are included.

Specify .NET Core SDK version for build

e20803d

Let dev choose to run tests in parallel or not

553b745

If it's in parallel then test discovery is also in parallel, which can really tank your CPU at IDE startup.

Remove redundant cast

72a0cb4

This is the only analysis warning that really throws OmniSharp for a loop on Mac. It can't figure out if System.Enum is in a missing assembly reference or what; this fixes it.

Revert accidental commit to properties.

bb2e7dd

tillig requested a review from a team March 9, 2022 01:02

reyang reviewed Mar 9, 2022

View reviewed changes

global.json Show resolved Hide resolved

reyang reviewed Mar 9, 2022

View reviewed changes

test/OpenTelemetry.Tests.Stress/OpenTelemetry.Tests.Stress.csproj Show resolved Hide resolved

cijothomas approved these changes Mar 9, 2022

View reviewed changes

reyang approved these changes Mar 9, 2022

View reviewed changes

pellared reviewed Mar 9, 2022

View reviewed changes

.gitignore Show resolved Hide resolved

.vscode/extensions.json Outdated Show resolved Hide resolved

.vscode/extensions.json Outdated Show resolved Hide resolved

.vscode/extensions.json Show resolved Hide resolved

.vscode/settings.json Outdated Show resolved Hide resolved

tillig added 3 commits March 9, 2022 07:16

Removed spelling, test ext; added Markdownlint.

917c478

Dev environment requires .NET 6.

2ae40d2

Use .NET 6 for dotnet-format build task.

610a525

Comment TargetFrameworks - list in desc order.

8c62389

pellared approved these changes Mar 9, 2022

View reviewed changes

Merge branch 'main' into vscode-build

bf24bb1

cijothomas merged commit 5255917 into open-telemetry:main Mar 9, 2022

tillig deleted the vscode-build branch March 9, 2022 17:47

alanwest reviewed Mar 9, 2022

View reviewed changes

tillig mentioned this pull request Mar 11, 2022

Fix analyzer issues in docs #3026

Merged

alanwest mentioned this pull request Apr 29, 2022

🍺 I'm in a love/hate relationship with VSCode 🍺 #3243

Closed

Kielek mentioned this pull request Sep 27, 2022

Add smoke tests signalfx/splunk-otel-dotnet#16

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve VS Code and cross-platform dev experience #2997

Improve VS Code and cross-platform dev experience #2997

tillig commented Mar 9, 2022

codecov bot commented Mar 9, 2022 •

edited

Loading

cijothomas left a comment

reyang left a comment •

edited

Loading

pellared left a comment •

edited

Loading

tillig commented Mar 9, 2022

pellared commented Mar 9, 2022

cijothomas commented Mar 9, 2022

alanwest left a comment •

edited

Loading

Improve VS Code and cross-platform dev experience #2997

Improve VS Code and cross-platform dev experience #2997

Conversation

tillig commented Mar 9, 2022

Changes

codecov bot commented Mar 9, 2022 • edited Loading

Codecov Report

cijothomas left a comment

Choose a reason for hiding this comment

reyang left a comment • edited Loading

Choose a reason for hiding this comment

pellared left a comment • edited Loading

Choose a reason for hiding this comment

tillig commented Mar 9, 2022

pellared commented Mar 9, 2022

cijothomas commented Mar 9, 2022

alanwest left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Mar 9, 2022 •

edited

Loading

reyang left a comment •

edited

Loading

pellared left a comment •

edited

Loading

alanwest left a comment •

edited

Loading