Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small fixes Import-DbaXESessionTemplate #4917

Merged
merged 2 commits into from
Jan 11, 2019
+4 −3
Merged

Small fixes Import-DbaXESessionTemplate #4917

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
986b392
Update Import-DbaXESessionTemplate.ps1
Jan 8, 2019
7266e35
Update Import-DbaXESessionTemplate.ps1
Jan 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions functions/Import-DbaXESessionTemplate.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,9 @@ function Import-DbaXESessionTemplate {
Stop-Function -Message "You must specify Path or Template."
}

if (($Path.Count -gt 1 -or $Template.Count -gt 1) -and (Test-Bound -ParameterName Template)) {
if (($Path.Count -gt 1 -or $Template.Count -gt 1) -and (Test-Bound -ParameterName Name)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify why this change is required?

Template is a valid parameter and as documentation states:

Specifies the name of one of the templates from the dbatools repository

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name is for the name of the XE session to be created.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I attached issue #4923 to it. This caught my attention while scanning the code trying to understand what is happening.
The warning message contradicts the code,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the message then Template should still be checked and the Name parameter simply added to the logic:

Suggested change
if (($Path.Count -gt 1 -or $Template.Count -gt 1) -and (Test-Bound -ParameterName Name)) {
if (($Path.Count -gt 1 -or $Template.Count -gt 1) -and (Test-Bound -ParameterName Template) -and (Test-Bound -ParameterName Name)) {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wsmelton

The message is: "Name cannot be specified with multiple files or templates because the Session will already exist."

Notice that the parameter -Path also specifies a template, but it needs the full path and the file extention. Apart from this, the parameter -Template has the same function.

Breaking it into pieces:
"Name cannot be specified" : (Test-Bound -ParameterName Name)
"multiple files or templates" : ($Path.Count -gt 1 -or $Template.Count -gt 1)
--> multiple files means $Path.Count -gt 1, sames goes for templates

For me it's still clear the developer just made a typo, using the wrong parameter. Very small issue.

To prove my case further, let me show this test:

Import-DbaXESessionTemplate -SqlInstance vmSQL -Path 'C:\Users\ijeb\Documents\GitHub\dbatools\bin\XEtemplates\Failing Queries.xml','C:\Users\ijeb\Documents\GitHub\dbatools\bin\XEtemplates\Function Executions.xml' -Name 'NewSession'

This should give the warning and abort, because it is not possible to have two sessions with the same name.

With the code I committed the result is:
WARNING: [08:23:07][Import-DbaXESessionTemplate] Name cannot be specified with multiple files or templates because the Session will already exist.

And with your suggested change (after deleting NewSession manually) the result is:

ComputerName : vmSQL
InstanceName : MSSQLSERVER
SqlInstance : vmSQL
Name : NewSession
Status : Stopped
StartTime :
AutoStart : False
State : Existing
Targets : {}
TargetFile : {}
Events : {sqlserver.error_reported}
MaxMemory : 4096
MaxEventSize : 0

WARNING: [08:27:03][Import-DbaXESessionTemplate] NewSession already exists on vmSQL

So no warning is shown. This is wrong.

However, today I thought of another possibility. It is possible to use the parameters -Path and -Template at the same time.

This should also show the warning, but it does not
Import-DbaXESessionTemplate -SqlInstance vmSQL -Path 'C:\Users\ijeb\Documents\GitHub\dbatools\bin\XEtemplates\Failing Queries.xml' -Template 'Function Executions' -Name 'NewSession'

So $Path.Count equals 1
$Template.Count equals 1

These two numbers need to be added together. The developer did not think of this case.

This can be solved

if ((($Path.Count + $Template.Count) -gt 1) -and (Test-Bound -ParameterName Name)) {

However, in this case the warning message is not good anymore. I would suggest:

It is not possible to import multiple templates together and give them the same name. Either import one template at a time or do not use the Name parameter.

What do you think of this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention is not for you to use Template with the Path parameter. Path is only used when you want to provide your own custom template. The Template path is used when you want to utilize the templates we include in the module (the command will iterate through and find the file that matches the Template name).

So you can use:

  • Name + Path
    Import-DbaXeSessionTemplate … -Path '<path to my xml file>' -Name MySession
  • Name + Template
    Import-DbaXeSessionTemplate … -Template 'Profiler Tuning' -Name MySession

Combining the following parameters should throw a message as invalid parameter combinations:

  • Path + Template
  • Path + Template + Name

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is leading nowhere.
My intention is to only change a small inconsistency in the code. I don't want to change the original design.
How should I clean this up in a clean way? Just delete the branch / revoke the PR?

Stop-Function -Message "Name cannot be specified with multiple files or templates because the Session will already exist."
return
}

foreach ($instance in $SqlInstance) {
Expand Down Expand Up @@ -191,7 +192,7 @@ function Import-DbaXESessionTemplate {
}

try {
Write-Message -Level Verbose -Message "Importing $file as $name "
Write-Message -Level Verbose -Message "Importing $file as $Name "
$session = $store.CreateSessionFromTemplate($Name, $file)
$session.Create()
if ($file -eq $tempfile) {
Expand All @@ -204,4 +205,4 @@ function Import-DbaXESessionTemplate {
}
}
}
}
}