Add support for vs code markers #3399
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
srdjanjovcic
requested review from
aortiz-msft,
cristinamanum,
dominoFire,
donnie-msft,
dtivel,
heng-liu,
kartheekp-ms,
nkolev92,
rrelyea,
zivkan and
zkat
as code owners
nkolev92
reviewed
May 19, 2020
There was a problem hiding this comment.
Looks good.
Can we add some basic tests please?
I see some stuff inhttps://github.com/NuGet/NuGet.Client/blob/dev/test/NuGet.Clients.Tests/NuGet.PackageManagement.VisualStudio.Test/Telemetry/NuGetTelemetryServiceTests.cs, but maybe there's better places too.
Will do. |
nkolev92
reviewed
May 19, 2020
|
This is how it looks like in ETW with this change. Events ending with /Start and /Stop are not sent to telemetry: |
Added unit tests for TelemetryActivity. |
|
You should rebase on top of latest dev because of ce4aa78 |
nkolev92
approved these changes
May 20, 2020
| @@ -59,3 +59,4 @@ | |||
| [assembly: SuppressMessage("Build", "CA1062:In externally visible method 'void OutputConsoleLogger.Log(ILogMessage message)', validate parameter 'message' is non-null before using it. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.VisualStudio.Common.OutputConsoleLogger.Log(NuGet.Common.ILogMessage)")] | |||
| [assembly: SuppressMessage("Build", "CA1823:Unused field 'LogEntrySource'.", Justification = "<Pending>", Scope = "member", Target = "~F:NuGet.VisualStudio.Common.OutputConsoleLogger.LogEntrySource")] | |||
| [assembly: SuppressMessage("Build", "CA1063:Provide an overridable implementation of Dispose(bool) on 'OutputConsoleLogger' or mark the type as sealed. A call to Dispose(false) should only clean up native resources. A call to Dispose(true) should clean up both managed and native resources.", Justification = "<Pending>", Scope = "type", Target = "~T:NuGet.VisualStudio.Common.OutputConsoleLogger")] | |||
| [assembly: SuppressMessage("Globalization", "CA1308:Normalize strings to uppercase", Justification = "Need to unify event names to be same as ones produced from telemetry.", Scope = "member", Target = "~M:NuGet.VisualStudio.NuGetVSTelemetryService.StartActivity(System.String)~System.IDisposable")] | |||
There was a problem hiding this comment.
this rule is getting annoying :)
I get why it's trying to promote and why it's less error prone, but there's so much infrastructure that depends normalizes to lowercase when the culture is known.
Thoughts @NuGet/nuget-client Should we disable this in a future PR?
There was a problem hiding this comment.
Yeah, I think it is a good candidate for repo-wide suppression 🙂
srdjanjovcic
force-pushed
the
dev-srdjanj-Add-support-for-VS-code-markers
branch
from
a331492 to
8ab0002
Compare
zivkan
approved these changes
May 20, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
Fixes: https://github.com/NuGet/Client.Engineering/issues/302
Regression: No
Implementation
Implements a new API on
INuGetTelemeteryServiceto log a telemetry marker, which is not sent as a telemetry event. Then, use the new API to emit a start code marker at the start of every activity.Testing/Validation
Tests Added: Unit tests to execute new code paths.
Reason for not adding tests: N/A
Validation: Validated that correct ETW events are emitted where expected.