Added telemetries #7474
Added telemetries #7474
Conversation
|
|
||
| [Parameter(ParameterSetName = "ApplicationParameterFilePath")] | ||
| [Parameter(ParameterSetName = "ApplicationName")] | ||
| [ref]$status |
There was a problem hiding this comment.
$status is good candidate global variable - then you won't need to pass along.
There was a problem hiding this comment.
Lets us PassByReference as its more controlled and always safer then to use Global variable
There was a problem hiding this comment.
Passing an extra optional parameter (with reference) still seems fine to me as there is no extra memory usage
There was a problem hiding this comment.
In this case there does not seem to be usual problems related to global variables. Before execution a command will set status and that will always be the truth - we don't have multi-threading or parallelization in our flow.
I am in favor of global as code will be cleaner without need to pass $status everywhere.
| @@ -142,6 +146,7 @@ | |||
|
|
|||
| if (!$SkipPackageValidation) | |||
| { | |||
| $status.value = "NewApp-TestingApplicationPackage" | |||
| $packageValidationSuccess = (Test-ServiceFabricApplicationPackage $AppPkgPathToUse) | |||
There was a problem hiding this comment.
Let's remove redundant Test-. Keep it only in deploy.ps1
There was a problem hiding this comment.
Will raise a separate PR for the same
There was a problem hiding this comment.
Please file an issue for that - otherwise we will miss it.
| if (!$?) | ||
| { | ||
| throw (Get-VstsLocString -Key SFSDK_UnableToUnregisterAppType) | ||
| } | ||
| $ApplicationTypeAlreadyRegistered = $false |
There was a problem hiding this comment.
Its a bug since !$? check for last command ran successfully or not and $ApplicationTypeAlreadyRegistered = $false is not the one for which we're checking for
| @@ -230,6 +236,7 @@ | |||
| # It will unregister the existing application's type and version even if its different from the application being created, | |||
| if ((Get-ServiceFabricApplication | Where-Object {$_.ApplicationTypeVersion -eq $($app.ApplicationTypeVersion) -and $_.ApplicationTypeName -eq $($app.ApplicationTypeName)}).Count -eq 0) | |||
| { | |||
| $status.value = "NewApp-UnregisteringApplicationType" | |||
There was a problem hiding this comment.
Rather than settings similar status multiple places, you can move it to separate method and call it from both New/upgrade file. I don't think adding "NewApp-" or "UpgradeApp-" prefix is necessary.
There was a problem hiding this comment.
Adding these prefixes will give us clear idea about the code flow without any need to look in other trace to decide whether code is in Create flow or Upgrade flow.
Lets keep it!
There was a problem hiding this comment.
Do we really need to know whether it is upgrade or new when lets say registering the package? This information seems to be redundant.
On the positive side we will be able to move code to common functions and re-use them.
There was a problem hiding this comment.
Removing it in the hope that we don't need to know whether code failed in CreateApp or UpgradeApp.
| } finally { | ||
| } | ||
| catch { | ||
| Write-Telemetry "Task_InternalError" "TaskStatus: $status | Exception: $_.Exception.GetType().FullName" |
There was a problem hiding this comment.
Remove words "TaskStatus:" and "Exception:". We can use convention to have status and exception type separated by | without any space.
There was a problem hiding this comment.
Used it for readability. If you don't see any use then lets remove it.
There was a problem hiding this comment.
It is redundant. As we will never need to see full string. We will always query this data via kusto and it is easier to write query without these extra characters
| } | ||
| catch { | ||
| Write-Telemetry "Task_InternalError" "TaskStatus: $status | Exception: $_.Exception.GetType().FullName" | ||
| } |
There was a problem hiding this comment.
Don't you need to re-throw?
|
|
||
| $global:operationId = $SF_Operations.Undefined |
There was a problem hiding this comment.
it is set to connect in Connect helper Connect-ServiceFabricClusterFromServiceEndpoint.ps1
| @@ -107,11 +111,13 @@ try { | |||
|
|
|||
| if (!$skipValidation) | |||
| { | |||
| $global:operationId = $SF_Operations.TestApplicationPackage | |||
There was a problem hiding this comment.
Should we add Get Application as operation as well
There was a problem hiding this comment.
Vinod did not add it initially. I am not sure whether he had a reason. But I agree ideally we should add.
There was a problem hiding this comment.
Added a few other missing as well
| } | ||
| $erroCode = ('"{0}":{1}' -f $erroCodeMsg, $errorMsg) | ||
| ## Form errorcode as json string | ||
| $erroCode = '{' + $erroCode + '}' |
There was a problem hiding this comment.
it actully is code as per design https://github.com/Microsoft/vsts-tasks/blob/master/docs/authoring/commands.md
| } | ||
| $erroCode = ('"{0}":{1}' -f $erroCodeMsg, $errorMsg) | ||
| ## Form errorcode as json string | ||
| $erroCode = '{' + $erroCode + '}' |
There was a problem hiding this comment.
This doesn't ensure json string if char escaping etc is required. Use native ways to genrate the json
| do | ||
| { | ||
| $exceptionTypes += $exception.GetType().FullName + ";" | ||
| $exception = $exception.InnerException |
There was a problem hiding this comment.
Consider handling AggregateException as well
* Added telemetries * Updating reference values * Updated code as per review comments * Adding a text message before force removing application package * Modified code as per review comments * re-factor telemetry to common code and re-use across all SF tasks * make.json changes * PR comments # Conflicts: # Tasks/ServiceFabricDeployV1/task.json # Tasks/ServiceFabricDeployV1/task.loc.json
* Added telemetries * Updating reference values * Updated code as per review comments * Adding a text message before force removing application package * Modified code as per review comments * re-factor telemetry to common code and re-use across all SF tasks * make.json changes * PR comments # Conflicts: # Tasks/ServiceFabricDeployV1/task.json # Tasks/ServiceFabricDeployV1/task.loc.json
No description provided.