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.

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

Add additional logging for notification conditions #516

Merged
merged 3 commits into from
Feb 16, 2019

Conversation

sghill
Copy link
Contributor

@sghill sghill commented Feb 14, 2019

  • Extracted Condition interface that let's us log both if the condition
    was met and if the user preference allowed the notification to be sent
  • Conditions that have interesting logic are unit-tested
  • Moves to the message supplier style of logging, which is not invoked
    unless that level of logging is enabled
  • Introduces a build key format - [ProjectFullDisplayName #] to
    better indicate which build the messaging is coming from

Fixes #508

@timja
Copy link
Member

timja commented Feb 14, 2019

Looks good, I'll try it out soon just to make sure it's fine

@timja
Copy link
Member

timja commented Feb 14, 2019

Here's an example log from running a build with all options ticked:

Feb 14, 2019 8:32:27 AM jenkins.plugins.slack.SlackNotifier prebuild
INFO: Performing start notifications
Feb 14, 2019 8:32:27 AM jenkins.plugins.slack.ActiveNotifier started
INFO: [freestyle 1 #4] was not caused by SCM Trigger
Feb 14, 2019 8:32:27 AM jenkins.plugins.slack.StandardSlackService publish
INFO: Posting succeeded
Feb 14, 2019 8:32:27 AM hudson.model.Run execute
INFO: freestyle 1 #4 main build action completed: SUCCESS
Feb 14, 2019 8:32:27 AM jenkins.plugins.slack.SlackNotifier perform
INFO: Performing complete notifications
Feb 14, 2019 8:32:27 AM jenkins.plugins.slack.ActiveNotifier completed
INFO: [freestyle 1 #4] found #3 as previous completed, non-aborted build
Feb 14, 2019 8:32:27 AM jenkins.plugins.slack.decisions.Condition test
INFO: [freestyle 1 #4] will send OnSuccessNotification because build matches and user preferences allow it
Feb 14, 2019 8:32:27 AM jenkins.plugins.slack.ActiveNotifier$MessageBuilder getStatusMessage
INFO: [freestyle 1 #4] selected #3 as previous completed, non-aborted build
Feb 14, 2019 8:32:28 AM jenkins.plugins.slack.StandardSlackService publish
INFO: Posting succeeded
Feb 14, 2019 8:32:28 AM jenkins.plugins.slack.SlackNotifier perform
INFO: Performing finalize notifications

@timja
Copy link
Member

timja commented Feb 14, 2019

@sghill What do you think about moving some of those messages to the build log instead of the system log?

I don't like how in freestyle projects its silent and you can't even tell its done anything

@timja
Copy link
Member

timja commented Feb 14, 2019

(Code is good except for the missing null checks around previous build and tests around that)

@sghill sghill force-pushed the additional-logging branch from dddcc47 to 9b36578 Compare February 15, 2019 02:30
@sghill
Copy link
Contributor Author

sghill commented Feb 15, 2019

Thanks for the quick review @timja!

I updated the Context to never have a nullable previous result (falls back to success, just like before). Tests have been added for this as well.

I don't like how in freestyle projects its silent and you can't even tell its done anything

I agree, that's a great idea.

What do you think about moving some of those messages to the build log instead of the system log?

I introduced a SystemAndUserLogger which will write info-level logs to both the build log and the system log. I'd prefer them in both places because it can be helpful to only have to look at the system log as an operator. This is also covered by tests.

Here's an example of logs in the build (they include the plugin name so we know where they came from):

[Slack Notifications] found #5 as previous completed, non-aborted build
[Slack Notifications] will send OnSuccessNotification because build matches and user preferences allow it
Finished: SUCCESS

I moved the lifecycle messages to the FINE level. Here's an example of the system log at INFO level:

[my-freestyle-job #6] found #5 as previous completed, non-aborted build
[my-freestyle-job #6] will send OnSuccessNotification because build matches and user preferences allow it

and at FINE level:

[my-freestyle-job #6] Performing start notifications
[my-freestyle-job #6] was not caused by SCM Trigger
[my-freestyle-job #6] Performing complete notifications
[my-freestyle-job #6] found #5 as previous completed, non-aborted build
[my-freestyle-job #6] does not match OnAbortedNotification condition
[my-freestyle-job #6] does not match OnSingleFailureNotification condition
[my-freestyle-job #6] does not match OnRepeatedFailureNotification condition
[my-freestyle-job #6] does not match OnNotBuiltNotification condition
[my-freestyle-job #6] does not match OnBackToNormalNotification condition
[my-freestyle-job #6] will send OnSuccessNotification because build matches and user preferences [my-freestyle-job #6] Performing finalize notifications

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

Code looks great, thanks, Will merge if manual testing passes

@timja timja self-assigned this Feb 15, 2019
@timja
Copy link
Member

timja commented Feb 15, 2019

Just the * imports and the question on the logger, otherwise code is good and will get 🚢'ed soon

- Extracted Condition interface that let's us log both if the condition
  was met and if the user preference allowed the notification to be sent
- Conditions that have interesting logic are unit-tested
- Moves to the message supplier style of logging, which is not invoked
  unless that level of logging is enabled
- Introduces a build key format - [ProjectFullDisplayName #<build>] to
  better indicate which build the messaging is coming from
- Introduce SlackFactory in ActiveNotifier instead of passing in the
  dependencies to create a slack factory method
- Introduce SlackNotificationsLogger - which writes to the system log
  with the build key embedded, and writes info-level messages to the
  build log with the plugin name embedded
@sghill sghill force-pushed the additional-logging branch from 9b36578 to 81af2d4 Compare February 15, 2019 21:48
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

LGTM, will merge after green build

@timja timja merged commit b3a9261 into jenkinsci:master Feb 16, 2019
@sghill sghill deleted the additional-logging branch February 18, 2019 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants