Telemetry for temp keys #3662
Telemetry for temp keys #3662
Conversation
chenriksson
requested review from
joelverhagen,
emgarten,
zhili1208,
dtivel,
anangaur,
skofman1 and
rrelyea
| var user = GetCurrentUser(); | ||
| await AuthenticationService.AddCredential(user, credential); | ||
|
|
||
| TelemetryService.TrackSymbolsPushEvent(id, version, user, User.Identity); |
There was a problem hiding this comment.
Although this API is intended to be used for symbol source, nothing in the code suggest that this is necessarily the case. I suggest continuing with that line in the event name, and to call it something like "CreatePackageVerificationKeyEvent" (or something shorter 😃 )
| @@ -245,11 +248,13 @@ private Credential GetCurrentCredential(User user) | |||
| { | |||
| await AuthenticationService.RemoveCredential(user, credential); | |||
| } | |||
|
|
|||
| TelemetryService.TrackSymbolsPushCallbackEvent(id, version, user, User.Identity, result?.StatusCode ?? 200); | |||
|
|
||
| var telemetryProperties = new Dictionary<string, string>(); | ||
| telemetryProperties.Add(AuthenticatinMethod, authenticationMethod); | ||
| telemetryProperties.Add(ClientVersion, clientVersion); | ||
| telemetryProperties.Add(PackageId, packageId); |
There was a problem hiding this comment.
I like the idea of shring code between the events, however the result here that events have properties they don't really need. For example: SymbolsPush event has property with account creation date. This information is useful for PackagePush event, because it will be used to analyze the usage of scoped api keys. Also, PackagePush event will have a prioperty "HasVerifyScope", which is unrelated to the push package scenario.
|
|
||
| return result; | ||
| return (ActionResult)result ?? new EmptyResult(); |
There was a problem hiding this comment.
Yes due to ??, since EmptyResult can't be cast as HttpStatusCodeWithBodyResult.
src/NuGetGallery/ExtensionMethods.cs
Outdated
| { | ||
| var scopeClaim = self.GetScopeClaim(); | ||
|
|
||
| return ScopeEvaluator.ScopeClaimsAllowsActionForSubject(scopeClaim, null, |
| public const string PackageVersion = "PackageVersion"; | ||
|
|
||
| // Verify package properties | ||
| public const string HasVerifyScope = "HasVerifyScope"; |
There was a problem hiding this comment.
Consider renaming HasVerifyScope to "IsVerificationKeyUsed". The usage of scopes in this scenario is an implementation detail..
…7.03.27 * tag 'v2017.03.27': (205 commits) Revert UpdateIsLatest optimistic concurrency changes (NuGet#3707) UpdateIsLatest concurrent unlist fix (NuGet#3695) Change telemetry time to use correct format (NuGet#3690) Fix typo of "publically" (NuGet#3636) Fix regression (NuGet#3667) Add credential to Register and RequestPasswordReset audits (NuGet#3666) Functional test for temp keys (NuGet#3664) Telemetry for temp keys (NuGet#3662) Temp keys implementation (NuGet#3563) (NuGet#3646) Extracting code: single type per file (NuGet#3644) Telemetry for package push (NuGet#3649) Upgrade to NuGet.* v4.0.0 dependencies (NuGet#3643) Fix concurrent push test by disabling search hijacking on feed (NuGet#3641) Fixing Package Description truncation (NuGet#3638) Fix Microsoft Account removal (NuGet#3639) Send e-mail when a new API key is created (NuGet#3634) IsLatest Fix: wrong connection string passed to retry context (NuGet#3632) Update WindowsAzure.Storage to 7.0.0 (NuGet#3633) Depend on signed version of Elmah (NuGet#3609) Move AzureEntityList and TableErrorLog to NuGetGallery.Core (NuGet#3607) ...
No description provided.