Skip to content
New issue

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

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

Already on GitHub? # to your account

feat: waifu log watcher #261

Closed
wants to merge 4 commits into from
Closed

Conversation

zaerald
Copy link
Member

@zaerald zaerald commented Oct 31, 2020

This PR implements watching of logs by keyword and triggers an alert once it's present in the runtime logs.

Demo

demo

Settings

Screen Shot 2020-10-31 at 19 11 15

I planned to add this to Events tab but this would always be changed by the users so it was added at the second tab, I also plan to enhance this into something more intuitive, like adding action and make it a list of keywords where any keywords present in logs would trigger an alert, and more intuitive if we can select from the logs and add it to the list. But for now, one keyword should suffice the use cases.

Some of the use cases:

  • Debugging - Most developers debug with classic System.out.println(), console.log(), and whatnot, these may not be the best debugging tool but it is one of the best ;). For instance, you've printed a log hacker: I'm in and add this keyword for waifu to watch and do your stuff without constantly checking the logs 😎
  • Standby startup - There are large applications where the startup time takes almost a minute (e.g. large Spring Boot application) where only logs state if it finished starting.

@zaerald zaerald added this to the WMP v1.4 milestone Oct 31, 2020
@zaerald zaerald requested a review from Unthrottled October 31, 2020 11:18
@zaerald zaerald linked an issue Oct 31, 2020 that may be closed by this pull request
@zaerald zaerald force-pushed the feat/WMP-225-waifu-log-watcher branch from 8fd5ae0 to b5eaba6 Compare October 31, 2020 11:30
@Unthrottled Unthrottled self-assigned this Oct 31, 2020
@@ -47,5 +47,10 @@
<option name="TAB_SIZE" value="2" />
</indentOptions>
</codeStyleSettings>
<codeStyleSettings language="kotlin">
<indentOptions>
<option name="CONTINUATION_INDENT_SIZE" value="4" />
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this to a tracked code style as ktlint gets mad at me as I was using 8 continuation indent.

@Unthrottled
Copy link
Member

Oh yeh, how do you want to handle branch updates? Do you want me to rebase when I merge other PRs?

@zaerald
Copy link
Member Author

zaerald commented Oct 31, 2020

It's fine for me if you want to do it, and it's also okay in my future PRs.

@Unthrottled
Copy link
Member

It's fine for me if you want to do it, and it's also okay in my future PRs.

I guess since you are here, could you update please? I'll try to rebase when I can in the future (it also might depend on the amount of merge conflicts too, I might make you do it then 😉 )

@zaerald zaerald force-pushed the feat/WMP-225-waifu-log-watcher branch from b5eaba6 to 08dff4c Compare October 31, 2020 12:46
@Unthrottled
Copy link
Member

make it a list of keywords where any keywords present in logs

Yes please, I would like this feature ⭐

more intuitive if we can select from the logs and add it to the list

It would be cool if it was like a right-click menu item like Add to waifu watch list 👓

Copy link
Member

@Unthrottled Unthrottled left a comment

Choose a reason for hiding this comment

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

I ❤️ this idea! Why stare at the logs when your waifu can do that for you?

I would like to see a solution the integration test issue though.

@@ -5,3 +5,11 @@ data class AlertConfiguration(
val isDisplayNotificationEnabled: Boolean = false,
val isSoundAlertEnabled: Boolean = false
)

object AlertConfigurationAllEnabled {
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about this?

data class AlertConfiguration(
    val isAlertEnabled: Boolean = false,
    val isDisplayNotificationEnabled: Boolean = false,
    val isSoundAlertEnabled: Boolean = false
) {
    companion object {
        fun allEnabled(): AlertConfiguration =
            AlertConfiguration(
                isAlertEnabled = true,
                isDisplayNotificationEnabled = true,
                isSoundAlertEnabled = true
            )
    }
}

Which in-turn enables AlertConfiguration.allEnabled()

@@ -145,6 +145,68 @@
</component>
</children>
</grid>
<grid id="21b3" layout-manager="GridLayoutManager" row-count="6" column-count="2" same-size-horizontally="false" same-size-vertically="false" hgap="-1" vgap="-1">
Copy link
Member

Choose a reason for hiding this comment

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

Should we be documenting the new features in the readme in the feature branches, or should that only happen when a release is cut?

Copy link
Member Author

@zaerald zaerald Nov 1, 2020

Choose a reason for hiding this comment

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

Oh right, if it's notable to our end users we should already include in PR for both README file and CHANGELOG, I'll update it.

private var alertShown = false

override fun getDefaultFilters(project: Project): Array<Filter> = when {
getPluginState().isLogWatcherEnabled -> arrayOf(
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really an issue yet, but something to keep in mind about motivation density. I've got two spring boot servers and am watching for started on port.

Peek 2020-10-31 08-20

Copy link
Member

Choose a reason for hiding this comment

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

I changed my mind, this might be an issue...
I forgot about spring boot tests

@ExtendWith(SpringExtension::class)
@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT,
		classes = [DemoApplication::class])

class DemoApplicationTests(
) {

	@Test
	fun contextLoads() {
	}

	@Test
	fun contextLoads2() {
	}

}

Just have a bunch of these guys running...

Peek 2020-10-31 08-33

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Peek 2020-10-31 09-44

I've got a temporary workaround, it will only do it once if I run it through the verify. But it might get annoying if I am debugging a test.

this.eventsBeforeFrustrationSpinner.setValue( this.state.getEventsBeforeFrustration() );
this.eventsBeforeFrustrationSpinner.setEnabled( allowFrustrationCheckBox.isSelected() );
this.frustrationProbabilitySlider.setValue( this.state.getProbabilityOfFrustration() );
this.frustrationProbabilitySlider.setEnabled( allowFrustrationCheckBox.isSelected() );
Copy link
Member

Choose a reason for hiding this comment

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

ninja code change

## Log Watcher
settings.tab.log.watcher=Log Watcher
settings.log.watcher.watch.logs=Watch Logs
settings.log.watcher.ignore.case.sensitivity=Ignore case sensitivity
Copy link
Member

Choose a reason for hiding this comment

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

IMO, I would just prefer it to be Ignore case

settings.tab.log.watcher=Log Watcher
settings.log.watcher.watch.logs=Watch Logs
settings.log.watcher.ignore.case.sensitivity=Ignore case sensitivity
settings.log.watcher.header=I'll watch the logs for you senpai!
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
settings.log.watcher.header=I'll watch the logs for you senpai!
settings.log.watcher.header=I'll watch the logs for you senpai! (๑•̀ㅂ•́)ง✧

image

😂

@zaerald zaerald marked this pull request as draft November 1, 2020 02:28
@zaerald
Copy link
Member Author

zaerald commented Nov 1, 2020

Converted to draft to work on the ff:

  • Document to README
  • Multiple alerts for multiple consoles session
  • Multiple alerts with Test integration
  • Document feature in update notification

@Unthrottled Unthrottled modified the milestones: WMP v1.4, WMP v1.5 Nov 5, 2020
@Unthrottled
Copy link
Member

So something that might make things a bit easier is if the log watcher is only active when a process is running and that process is not a test, wdyt?

@zaerald
Copy link
Member Author

zaerald commented Dec 14, 2020

The issue might still show up if for what you've demo wherein there are more than 1 running process which is not a test. For instance, I'll start the application (1 proc), then I'll start a different maven run (2nd proc).

I'll set a time for finishing this up 🤞, it gets a bit busy before the holidays 🎅

@Unthrottled
Copy link
Member

The issue might still show up if for what you've demo wherein there are more than 1 running process which is not a test.

While this is still an issue, only one notification will be present at a time avoiding this issue. See this comment for more details about notification collisions

Peek 2020-10-31 08-33

I haven't used the feature as much as I have wanted over time (I haven't had the need to look at logs in a bit). So it may still have some weird edge cases I haven't observed!

@Unthrottled Unthrottled deleted the feat/WMP-225-waifu-log-watcher branch January 31, 2021 23:40
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Waifu Log Watcher
2 participants