4
\$\begingroup\$

This is my first PowerShell script. It reads the event logs of several computers in the local network and lists error/warning messages written to them in the last 24 hours, if any.

I'll appreciate any suggestion to improve my code.

$computers = 'COMP1','COMP2','COMP3','COMP4'
$logs = 'Log1','Log2','Log3','Log4'
$yesterday = (Get-Date).AddDays(-1)

foreach ($comp in $computers) {
    try {
        $events = Get-WinEvent @{logname=$logs; starttime=$yesterday; level=2,3} -ComputerName $comp -ErrorAction Ignore;

        if ($events.count -gt 0) {
            $count = $events.count; 
            "$comp $count event(s):"; 
            $events | select logname, providername, timecreated, leveldisplayname, message `
                    | Format-Table -AutoSize;
        } else {
            "$comp no events";
        }
    }
    catch {
        "$comp error!!! $_";
    }
}
\$\endgroup\$
4
\$\begingroup\$

Basics

Aliases

PowerShell is meant to be both shell and programming language. As such, it has a concept of Aliases. An Alias is another name for a command. An alias can resolve to a cmdlet, a function, or even a native application. You can create your own aliases, but there are already many built in.

Aliases are great for quickly typing out commands in the shell or prototyping, but I and many others strongly believe you should avoid using them in scripts.

Try to use the full name of each command, as this can reduce ambiguity and increase code clarity.

In your script you only use one: select

This is an alias to Select-Object.

Parameters

Many (most) PowerShell commands accept parameters both as named parameters (MyCommand -ParamName ParamValue) and positional parameters (MyCommand ParamValue).

Similarly to the preference for using full command names, it is recommended to always use named parameters. In your script for example, so you are doing select logname, providername, timecreated (truncating the rest).

What you may not realize is that you are passing an array to a single parameter, and that parameter does in fact have a name: Property (the same is true for your first parameter to Get-WinEvent, whose first parameter in your case is called FilterHashTable).

Expanded out:

$events | Select-Object -Property logname,providername,timecreated

Although not in use in your script, you should be aware of the following:

  • Parameter names can also have aliases. So for example Get-WinEvent -ComputerName $comp could also be written Get-WinEvent -cn $comp. Aliases have to be defined; they aren't automatic.
  • Parameter names accept any unambiguous stub of the name. So, you can use a partial name, like Get-WinEvent -FilterH @{}. In this example, you can't use Get-WinEvent -Filter because there are also -FilterXml and -FilterXPath parameters, but you can do down to a single character as long as it's unambiguous for that command.

Both of these are good to know, but again should be avoided in scripts.

Format- commands

PowerShell makes heavy use of pipelines, but unlike most shells, PowerShell sends complete objects throughout its pipeline.

This is a great thing, and it's what allows you to do complex filtering and manipulation.

But when you see the output on the screen and it shows you a table or list of information, it's easy to think just think of it as formatted text.

PowerShell has Format- cmdlets that let you display information explicitly the way you want (since normally the host chooses a format based on the data types or amount of information).

Here's the really important thing to remember: Format- cmdlets are for end-result, final display only.

Once you send an object through a Format- cmdlet, it is no longer the original object; you've lost the original.

In almost all cases, it's best to send the original object, then let the caller or use decide what they want to do with it (whether it's format it, convert it to another data type, save is as a CSV, or whatever).

It's hard to tell what you want to do with the data in your script, but since you use Format-Table you've already excluded all of your other options.

Backtick ` as line continuation character

The backtick ` is the escape character in PowerShell. The standard backslash \ used in most languages is not used here because backslash is also the path separator on Windows, and since PowerShell is supposed to work as a shell and was born on Windows, this would be inconvenient. The backtick was chosen instead.

When you use backtick at the end of a line, you are in essence "escaping" the newline for the purposes of the parser.

The unfortunate thing is that the backtick is very small, and very difficult to see, so it can make things confusing.

I highly recommend that you avoid the backtick for line continuation at all costs.

Luckily, you do have some other options, as there are other points in commands and statements where you can start a newline without the need for any special character.

Pipe |

Since pipes are so common in PowerShell and you may be doing a pipeline that includes several statements, it's very useful to know that you can use a newline after a pipe (but not before). So the following works without any line continuation:

$events |
    Select-Object -Property logname,providername |
    Sort-Object -Property logname

(indentation after the first pipe is a stylistic choice, not at all required)

Operators

You can use a newline directly after an operator:

if ($var -gt 5 -and
    $var -lt 100) {
    # code
}

For long conditionals, this is essential.

ScriptBlocks {}

Any scriptblock is essentially a whole script, so you can use newlines liberally inside it, even if you were passing the scriptblock as a parameter. As soon as the scriptblock is opened, go for it!

Invoke-Command -ScriptBlock {
    # A whole new script here
    Get-Service |
        Get-Random |
        Stop-Service -Verbose -WhatIf
}

In grouping expressions () and sub-expressions $()

Using Grouping and Sub expressions even where not necessary can help if you feel the code needs better formatting. You can use newlines inside them as needed:

Get-Service -Name (
    'Win*'
)

Get-Service -Name (
    "Win$(
        '*'
    )"
)

Those examples are convoluted, but you get the idea.

In array/hashtable definitions @()/@{}

I use these a lot. Format as needed within these. Note that in a hashtable definition, if you are used to using semicolons ; to separate items on a single line (like @{ Key1 = 'val1' ; Key2 = 'val2' } the newlines take the place of the semicolon.

# Hashtable
@{
    Key1 = 'val1'
    Key2 = 'val2'
}

# Array
@(
    1,2,3
)

@(
    1,
    2,
    3
)

@( 
    1 ,
    2
    ,3
) # why oh why

# my preferred (long) array style:
@(
     1
    ,2
    ,3
)

Combined:

@{
    Set1 = @{
        Arr1 = @(
             'A'
            ,'B'
            ,'C'
        )

        Arr2 = @(
             1
            ,2
            ,3
        )
    }

    Set2 = @{
        Scalar = 10
    }
}

In a command:

Get-WinEvent -FilterHashTable @{
    logname = $logs
    starttime = $yesterday
    level = @(
         2
        ,3
    ) # I wouldn't expand the array like this for 2 elements, just an examples
} -ComputerName $comp

Errors and Exceptions

PowerShell has a concept of terminating vs. non-terminating errors. Different commands and different code determine whether an error stops execution or whether it just reports an error and keeps going. Most commands allow you to override whatever their default is with the -ErrorAction common parameter.

In your code, you are using try/catch, but you've added -ErrorAction Ignore to Get-WinEvent.

Errors that are non-terminating, which includes an error you've told PowerShell to ignore or ContinueSilently, will not trigger a try/catch, so I believe your catch block will never run even if there is an error.

Instead, you probably want -ErrorAction Stop, which is supposed to force all errors to become terminating errors, so that they do trigger the exception handling.

Truthiness and Falsiness

PowerShell tries pretty hard to convert or coerce values into the type that is required. In the case of a boolean value, like that used in an if statement, almost anything can be interpreted as true or false.

The following all evaluate to $false with no additional action or conversion needed:

  • $false
  • 0
  • $null
  • Empty string
  • Empty array
  • Uninitialized variable (not the same as one containing $null)

Just about everything else, including, confusingly, an empty hashtable, is interpreted as $true.

So what does this mean for you?

if ($events.count -gt 0)

Can be reduced to:

if ($events.count)

Which could even further be reduced to:

if ($events)

This is useful to check for lots of things; return values of course, but also whether a parameter contains a value or is otherwise available.

Pipeline Output

When you do something like this:

"Some thing"

You're used to it showing up on the screen, so it's easy to think that this is the same as a sort of print statement (which in PowerShell would be Write-Host).

But that isn't what's happening; it's actually implicitly calling Write-Output, which sends that to the output stream.

If the output stream makes its way back to the host, then the host decides what to do with it; in the case of powershell.exe (or ISE) it decides to display it, but in the case of a function for example, this becomes part of the return value. In the case of a status message that's not really what you want.

Here's an example:

function Test-Output1 {
    "I'm returning 5"
    5
}

function Test-Output2 {
    Write-Host -Object "I'm returning 5"
    5
}

Now, if you run these directly:

Test-Output1
Test-Output2

You will see the exact same output. They are indistinguishable that way.

But try to assign the result to a variable instead:

$v1 = Test-Output1
$v2 = Test-Output2

Now, you will see the difference. Test-Output1 shows nothing on the screen. If you check the value of $v1, it will be an array, with 2 elements. The first element is a string (your message), the second element is the number.

Test-Output2 on the other hand showed a message anyway, and the value of $v2 is only the number.

That's the critical difference.

So even if you're in a situation where you can't see the difference, you should always be thinking about it. "Am I trying to show a message to a person on the screen, or am I trying to return a value to somewhere else in the program?"

Details

Ok, so now some stuff that actually has to do with the task you're trying to achieve!

Parameters

This is something of a general language element thing too, but instead of hardcoding variables at the top of your script, consider adding parameters to the script so that as a caller you can specify the values you want.

Let's look at replacing the first 3 lines of your script:

$computers = 'COMP1','COMP2','COMP3','COMP4'
$logs = 'Log1','Log2','Log3','Log4'
$yesterday = (Get-Date).AddDays(-1)

With this:

[CmdletBinding()]
param(
    $Computers,
    $Logs,
    $StartTime
)

Now, I renamed $yesterday to $StartTime so that you can specify the date you want when calling, so that variable would have to change, but the entire rest of your script would work the same. If your script is named Get-MyLogs.ps1 then you can call it like this:

.\Get-MyLogs.ps1 -Computers 'COMP1','COMP3' -Logs 'Log1','Log9999' -StartTime (Get-Date).AddDays(-1)

But maybe you almost always want to use "yesterday" and it's too annoying to enter that value every time. You can provide your parameter with a default value instead:

[CmdletBinding()]
param(
    $Computers,
    $Logs,
    $StartTime = $((Get-Date).AddDays(-1))
)

Now you can leave that parameter off, and it will use the default, but you can still explicitly set a value to override it. This gives you a great amount of flexibility, without having to edit the script, and it required almost no effort.

I didn't even go into adding data types to the parameters, validation, etc.

I will only add that there are some naming conventions you should follow for parameters.

Typically plural is not used, so even if you want to accept multiple logs, you would probably name that parameter $Log and not $Logs.

Accepting a computer name is common and the parameter is pretty much always called $ComputerName (again, even if multiple names are accepted). It's good to follow these conventions. You can add aliases to your parameters if you want to support non-standard additional names, but real name should generally follow convention when possible.

Parallelization

Right now, you go through the list of computers sequentially. For more than a few computers, this is going to be slow. If any of the computers are not available, and have to time out, it's going to be really slow.

Before going further, I'll just say this: if it's ok that it's slow, then stop here. Let it be slow. Sequential, non-parallelized code is easy to write, easy to understand. If this just runs as a background task or whatever, don't complicate it.

But if you want to check multiple computers at once, you can.

PowerShell sucks at this, but there are options.

The built-in ways on Windows are Workflows and Jobs.

Workflows are weird. They're a bit complicated, difficult to understand all the implications, and hard to find good help on, because nobody uses them. They are also not going to be included in PowerShell core since they only work on Windows so I don't recommend this. For very simple things, it might be a good way to go.

Jobs are supported through the *-Job* cmdlets, and the -AsJob parameter that's available on some commands.

Jobs actually run as separate PowerShell processes so they are a bit expensive in terms of start time and resource usage (especially with large numbers of them). Also because they are not in-process, any objects passed between them are serialized/deserialzed so some functionality may be lost.

If you start a scriptblock in a job, you then have a job object which you can wait on, check the progress on, and then when ready receive its output.

There are more advanced ways to do parallel workflows like runspaces, which you can do yourself, or use a library like Boe Prox's excellent PoshRsJob module.

I'm not going to get any deeper on this since I don't even know your intention and there's so much to write. If you try to parallelize this, try some stuff, try different methods, and use Stack Overflow as needed.

Conclusion

Despite my novel, your code is pretty straightforward. Without knowing how you intend to use the data it's hard to be opinionated on any deeper design changes, so clearly most of my suggestions are general language stuff.

\$\endgroup\$
  • 1
    \$\begingroup\$ This is like a tutorial - amazing, I'll need to read it a few times ;-) \$\endgroup\$ – t3chb0t Nov 12 '17 at 18:21
  • 2
    \$\begingroup\$ @t3chb0t I'll send you a Kindle version, good for long plane rides ;-p \$\endgroup\$ – briantist Nov 12 '17 at 18:27
  • 1
    \$\begingroup\$ This is a masterpiece of an answer, bravo! \$\endgroup\$ – Daniel Nov 12 '17 at 19:40
  • \$\begingroup\$ @briantist Thank you for your detailed answer, it helped me a lot! I've modified my code based on most of your suggestions. One thing though - I added -ErrorAction Ignore because not all logs are registered on all computers and I wanted to avoid the "there is not an event log on the computer that matches '...'" messages. I know it may not be the best way to achieve it, but I couldn't find a better way. And by the way, catch is reachable - e.g. if one of the computer is unavailable it will print "the RPC server is unavailable", etc. >>> \$\endgroup\$ – kodkod Nov 13 '17 at 15:26
  • \$\begingroup\$ >>> As for your question - for now the code is intended to only display the results on screen. But in the future I may try to create a scheduled task that will run the script routinely and send me an e-mail with its results. \$\endgroup\$ – kodkod Nov 13 '17 at 15:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.