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

Jump to bottom

Start of a PSES log file analyzer #797

Merged
merged 8 commits into from
Dec 3, 2018
Merged

Start of a PSES log file analyzer #797

merged 8 commits into from
Dec 3, 2018

Conversation

rkeithhill
Copy link
Contributor

Wanted to get some input on this one before I take it any further -
particularly the suggested logging change to MessageReader/Writer.

rjmholt
rjmholt reviewed Nov 26, 2018
View reviewed changes
Copy link
Contributor

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

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

Some quick comments on the C# code, will look at the PowerShell this week

src/PowerShellEditorServices.Protocol/MessageProtocol/MessageReader.cs Outdated Show resolved Hide resolved
src/PowerShellEditorServices.Protocol/MessageProtocol/MessageReader.cs Outdated Show resolved Hide resolved
src/PowerShellEditorServices.Protocol/MessageProtocol/MessageReader.cs Outdated Show resolved Hide resolved
@rkeithhill rkeithhill force-pushed the rkeithhill/pses-log-analyzer branch from bc5b6ce to 1cc9e57 Compare November 28, 2018 05:44
@rkeithhill rkeithhill changed the title WIP: Start of a PSES log file analyzer Start of a PSES log file analyzer Nov 29, 2018
@TylerLeonhardt
Copy link
Member

Can you give an example of how to use this? I'd like a bit more context

@rkeithhill
Copy link
Contributor Author

If you look in the Teams channel you'll see a bunch of examples of usage.

rjmholt
rjmholt reviewed Nov 30, 2018
View reviewed changes
tools/PsesLogAnalyzer/Parse-PsesLog.ps1

$message = parseLogMessage $method

[PsesLogEntry]::new($script:logEntryIndex, $timestamp, $timestampStr, $logLevel, $threadId, $method,
Copy link
Contributor

Choose a reason for hiding this comment

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

This module targets PS v5.1+ right? Might be worth setting the manifest field. Otherwise, need New-Object

Copy link
Contributor

Choose a reason for hiding this comment

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

Better yet, you could use the coercion syntax:

[PsesLogEntry]@{
    Index = $script:logEntryIndex
    Timestamp = $timestampStr
    TimestampStr = [datetime]$timestampStr
   ...
}

You should even get field completions for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll set the module manifest to require v5 or higher. This tool is primarily for project maintainers so I don't think that should be a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I set it to 5.1.

tools/PsesLogAnalyzer/Parse-PsesLog.ps1
$script:logEntryIndex++
}

function parseLogMessage([string]$Method) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not a param block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not using this like an advanced function (no need to support pipeline input, should process, etc).

tools/PsesLogAnalyzer/PsesLogAnalyzer.psd1 Outdated
# Description = ''

# Minimum version of the PowerShell engine required by this module
# PowerShellVersion = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Given class usage, this should be "5.1" I'm thinking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We got class support in 5.0, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so, but I think there a number of problems with 5.0 that have lead to its widespread removal and de facto deprecation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I set it to 5.1.

tools/PsesLogAnalyzer/PsesLogAnalyzer.psm1 Outdated
. $PSScriptRoot\Parse-PsesLog.ps1
. $PSScriptRoot\Analyze.ps1

Export-ModuleMember -Function *-*
Copy link
Contributor

Choose a reason for hiding this comment

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

@daxian-dbw thinks that this should be unneeded with FunctionsToExport set in the manifest.

Apparently the flow is:

defined commands --> Export-ModuleMember --> FunctionsToExport --> actual exported commands

Where --> is "filter through".

Copy link
Contributor

Choose a reason for hiding this comment

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

However, this bodes experimentation

Copy link
Contributor

Choose a reason for hiding this comment

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

Certainly defining an explicit list of exported functions is something we want to encourage for performance and discovery/analysis reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

tools/PsesLogAnalyzer/Types.ps1 Outdated
@@ -0,0 +1,152 @@
enum PsesLogLevel {
Diagnostic;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the semicolons needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Bad habit from when I defined simple enums on a single line. I'll fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@rkeithhill rkeithhill force-pushed the rkeithhill/pses-log-analyzer branch from 1f07039 to e35b7d2 Compare November 30, 2018 23:48
@rkeithhill
Copy link
Contributor Author

I think I've addressed all the PR feedback.

rjmholt
rjmholt approved these changes Dec 2, 2018
View reviewed changes
Copy link
Contributor

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

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

LGTM! I left two small comments, but they are just comments

tools/PsesLogAnalyzer/Parse-PsesLog.ps1
return $result
}

if (($Method -eq 'ReadMessage') -and
Copy link
Contributor

Choose a reason for hiding this comment

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

Given a cascade of if/elseif/else statements followed by a single implicit return, this might be refactorable as a series of if (...) { return } statements for slightly lower complexity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bigger change. I'll adress it later (in another PR) when I'm back from New York.

tools/PsesLogAnalyzer/Parse-PsesLog.ps1 Outdated
parseLogMessageBody -Discard
return $result
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

else here is not needed, since if always returns

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@rkeithhill
Copy link
Contributor Author

I'll be travelling tomorrow. Could someone merge this before 1.9.1 if @TylerLeonhardt approves it. Thanks.

TylerLeonhardt
TylerLeonhardt approved these changes Dec 3, 2018
View reviewed changes
Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

This looks great @rkeithhill! I can't wait to give it a try 👍 I +1'd the remaining feedback from @rjmholt.

@rkeithhill rkeithhill merged commit 65d8e70 into master Dec 3, 2018
@rkeithhill rkeithhill deleted the rkeithhill/pses-log-analyzer branch December 3, 2018 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjmholt rjmholt rjmholt approved these changes

@TylerLeonhardt TylerLeonhardt TylerLeonhardt approved these changes

@SeeminglyScience SeeminglyScience Awaiting requested review from SeeminglyScience

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rkeithhill @TylerLeonhardt @rjmholt