Report first-time install metric to Particular when a user installs NServiceBus #4280

indualagarsamy · 2016-11-08T00:44:11Z

Relates to:
https://github.com/Particular/OptimizeTrialExtensions/issues/114
#4262
https://github.com/Particular/Operations.Website.Backend/pull/237

Release Notes

We have removed the page that pops up after installing the NServiceBus NuGet package as the general consensus is that developers don't like it. Examples:

However the page served an alternate purpose which was to indicate when someone has installed the NuGet package for the first time, information we use to determine if we are reaching developers with our message. In place of the popup page, we have added code to the installation PowerShell script that sends a web request to our server to indicate that a first-time installation has occurred to that we can retain this statistic while still removing the post-installation pop-up page.

In, the new reworked PS Script, the majority of the script is now run as a background job. The advantages to this approach are:

Background jobs run async to the main script
Background jobs run silently even if they fail
Also, it will be easier to debug any issues encountered in the script. Tracing is also turned on in the background job so the execution path is visible. The job output can be obtained by using:

  Receive-Job -name particular.analytics

Pros:

Makes sure this metric report is only done inside a development environment.
Can continue to use the pre-existing google analytics that tracks installs as opposed to first time execution

Cons:

We are continuing to use and add more stuff to init.ps1 when we are actually trying to get rid of it.
Creates more problems, see: init.ps1 freezing with VS2015 #4235
This approach is bad for the long term. The latest nuget has already deprecated install/uninstall scripts
While the init script is technically still supported for now, the future remains unclear. Also, when installing the nuget into a project.json-based project, the install/uninstall scripts will be ignored.

@gbiellem @andreasohlund - please have a look.

…es/114

indualagarsamy · 2016-11-08T06:49:45Z

@gbiellem - i'm done. Please review. If happy, go ahead and remove the WIP.

danielmarbach · 2016-11-08T12:33:52Z

packaging/nuget/tools/init.ps1

 } 
 Catch [Exception] { 
 	Write-Warning $error[0]
 }
+finally {
+	if ($wc){
+	$wc.Dispose()


Is this guaranteed to work? According to the doco upload async asynchronously uploads without blocking which means the finally block can be executed during the upload.

@danielmarbach - See http://stackoverflow.com/questions/1999858/how-bad-is-it-to-not-dispose-in-powershell

As far as I know, the call will execute asynchronously, even after the package manager is done and back to the console. I tested this by running the backend locally and set a breakpoint. The nuget install completed and got back to the prompt but the code was still invoked and waiting at the breakpoint I set.

Also this is a one time thing. We are not calling this script over and over. IMO, it shouldn't have any impact. What exact changes are you requesting here? Do you have any suggestions of what I can add here in the powershell?

@gbiellem - your thoughts on this?

I think what @danielmarbach is getting at is since the powershell script is never awaiting the result of the async UploadValuesAsync call, that means the finally block could be executed and dispose the WebClient before the call finishes.

Correct what @bording said

timbussmann · 2016-11-08T15:52:34Z

packaging/nuget/tools/init.ps1

+	$postData.Add("version", $packageversion)
+	$wc = New-Object System.Net.WebClient
+	$wc.UseDefaultCredentials = $true
+	$wc.UploadValuesAsync($url,"post", $postdata)


since we don't catch the exception here, what happens with the script if an exception is thrown? is that swallowed the same way as in a c# app or will this break the script?

@timbussmann - I have tested this scenario with a bad url. It does not break the script.
I also tested it by disabling the network adapter and running the nuget install (from local source), errors do not get reported.

gbiellem · 2016-11-15T01:41:23Z

@indualagarsamy

I reworked the PS Script as you requested. The way it works now is the majority of the script is run as a background job. The advantages to this approach are:

background jobs run async to the main script
background jobs run silently even if they fail

I'm hoping that as well as addressing what you wanted it may also address the hanging issue in #4235. Since we can't reproduce that issue it's hard to know for sure.

I've also turned tracing on in the background job so you can see the execution path.
You can do this by dumping the job output using

Receive-Job -name particular.analytics

You'll see in the script if the Job exists in the current PS session the script skips it so for testing you'll need to remove the job between runs...

Remove-Job -name particular.analytics

If you're not familiar with PS Jobs take a look at https://technet.microsoft.com/en-us/library/hh847783(v=wps.620).aspx

I've made a few other tweaks to simplify it. Please smoke test it and see what you think

timbussmann · 2016-11-15T12:45:40Z

@indualagarsamy please remove the WIP prefix if you think this one is good to go now.

gbiellem · 2016-11-16T07:13:14Z

@timbussmann It's not ready. @indualagarsamy and I found an issue when smoke testing it.
We're working on it now

indualagarsamy · 2016-11-17T03:46:03Z

@Particular/nservicebus-maintainers - Both @gbiellem and I have tested this PS script. The only change we made in addition to @gbiellem's previous changes were to ensure that we don't display the notice message to an existing user if the user simply uninstalls/reinstalls the nuget package.

We've tested this end to end. This should address @danielmarbach's original concern of disposing the httpclient. We are now using the above approach to keep the installing the package completely asynchronous in relation to making the web request call.

Please review and merge.

timbussmann · 2016-11-17T10:09:12Z

packaging/nuget/tools/init.ps1

+    $platformKeyPath = 'HKCU:SOFTWARE\ParticularSoftware'
+
+    if ((Test-Path $nserviceBusKeyPath) -or (Test-Path $platformKeyPath)) {
+        New-Event -SourceIdentifier $noticeEvent -Sender "analytics" -MessageData "existing"


that event doesn't seem to be used, is this still required?

Yes it's used, without it the wait-event at the end of the script would have to wait for the timeout

outdated

timbussmann

looks good based on my very limited PS knowledge

indualagarsamy · 2016-11-17T23:11:31Z

@Particular/nservicebus-maintainers - The build is failing on a code inspection check. However, this PR has not made any code changes other than the powershell script. Can you please have a look.
@andreasohlund - it might be that the fix to app_packages isn't working. See below:

NServiceBus.Core/App_Packages/Particular.Licensing
 UserSidChecker.cs (1)
10: Expression is always true

Concern has been addressed

bording

LGTM

Relates to Particular/NServiceBus#4280 Relates to #193

* Privacy policy inclusions Relates to Particular/NServiceBus#4280 Relates to #193 * removed the last updated timestamp

indualagarsamy added 2 commits November 7, 2016 16:37

Relates to https://github.com/Particular/OptimizeTrialExtensions/issu…

bdbb160

…es/114

final tweaks after testing

40fed24

timbussmann previously approved these changes Nov 8, 2016

View reviewed changes

andreasohlund approved these changes Nov 8, 2016

View reviewed changes

danielmarbach previously requested changes Nov 8, 2016

View reviewed changes

timbussmann changed the title ~~WIP: Reporting first time install metric to Particular when a first time user installs NServiceBus~~ [WIP] Reporting first time install metric to Particular when a first time user installs NServiceBus Nov 8, 2016

timbussmann reviewed Nov 8, 2016

View reviewed changes

Changing the api to reflect the install metric and not the usage metric.

fb0722b

indualagarsamy added this to the 6.1.0 milestone Nov 9, 2016

indualagarsamy self-assigned this Nov 9, 2016

Remove UploadValuesAsync and convert to background job

57a1c62

andreasohlund approved these changes Nov 15, 2016

View reviewed changes

Make sure that the user is notified in the console.

b965195

Greg Bielleman and others added 3 commits November 17, 2016 14:18

Rework to show text only once

750905f

Update notice text

b4b873e

cleanup

06a262c

indualagarsamy changed the title ~~[WIP] Reporting first time install metric to Particular when a first time user installs NServiceBus~~ Reporting first time install metric to Particular when a first time user installs NServiceBus Nov 17, 2016

timbussmann reviewed Nov 17, 2016

View reviewed changes

timbussmann approved these changes Nov 17, 2016

View reviewed changes

timbussmann mentioned this pull request Nov 17, 2016

init.ps1 freezing with VS2015 #4235

Closed

Clean up extra blank Lines

b32adb9

bording approved these changes Nov 17, 2016

View reviewed changes

bording changed the title ~~Reporting first time install metric to Particular when a first time user installs NServiceBus~~ Report first-time install metric to Particular when a user installs NServiceBus Nov 17, 2016

bording merged commit c0a0118 into develop Nov 17, 2016

bording deleted the powershell-is-back branch November 17, 2016 23:57

This was referenced Nov 22, 2016

Update License agreement in the installer. Particular/ServiceInsight#635

Closed

Update License Agreement Particular/ServiceControl#856

Closed

Update License Agreement Particular/ServicePulse#385

Closed

danielmarbach mentioned this pull request Dec 8, 2016

Release 6.1.0 #4377

Merged

38 tasks

indualagarsamy pushed a commit to Particular/PlatformInstaller that referenced this pull request Dec 8, 2016

Privacy policy inclusions

6b299b3

Relates to Particular/NServiceBus#4280 Relates to #193

indualagarsamy mentioned this pull request Dec 8, 2016

Updated License Agreement Particular/PlatformInstaller#194

Merged

gbiellem pushed a commit to Particular/PlatformInstaller that referenced this pull request Dec 13, 2016

Updated License Agreement (#194)

e74d0a7

* Privacy policy inclusions Relates to Particular/NServiceBus#4280 Relates to #193 * removed the last updated timestamp

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report first-time install metric to Particular when a user installs NServiceBus #4280

Report first-time install metric to Particular when a user installs NServiceBus #4280

indualagarsamy commented Nov 8, 2016 •

edited

indualagarsamy commented Nov 8, 2016

danielmarbach Nov 8, 2016

indualagarsamy Nov 9, 2016 •

edited

bording Nov 9, 2016

danielmarbach Nov 10, 2016

timbussmann Nov 8, 2016

indualagarsamy Nov 9, 2016

gbiellem commented Nov 15, 2016

timbussmann commented Nov 15, 2016

gbiellem commented Nov 16, 2016

indualagarsamy commented Nov 17, 2016

timbussmann Nov 17, 2016

gbiellem Nov 17, 2016

timbussmann left a comment

indualagarsamy commented Nov 17, 2016

bording left a comment

Report first-time install metric to Particular when a user installs NServiceBus #4280

Report first-time install metric to Particular when a user installs NServiceBus #4280

Conversation

indualagarsamy commented Nov 8, 2016 • edited

Release Notes

indualagarsamy commented Nov 8, 2016

danielmarbach Nov 8, 2016

Choose a reason for hiding this comment

indualagarsamy Nov 9, 2016 • edited

Choose a reason for hiding this comment

bording Nov 9, 2016

Choose a reason for hiding this comment

danielmarbach Nov 10, 2016

Choose a reason for hiding this comment

timbussmann Nov 8, 2016

Choose a reason for hiding this comment

indualagarsamy Nov 9, 2016

Choose a reason for hiding this comment

gbiellem commented Nov 15, 2016

timbussmann commented Nov 15, 2016

gbiellem commented Nov 16, 2016

indualagarsamy commented Nov 17, 2016

timbussmann Nov 17, 2016

Choose a reason for hiding this comment

gbiellem Nov 17, 2016

Choose a reason for hiding this comment

timbussmann left a comment

Choose a reason for hiding this comment

indualagarsamy commented Nov 17, 2016

bording left a comment

Choose a reason for hiding this comment

indualagarsamy commented Nov 8, 2016 •

edited

indualagarsamy Nov 9, 2016 •

edited