Avoid sending deleted events for already completed jobs#133
Conversation
| return; | ||
| } | ||
|
|
||
| // If the build already has a result, this could be a Jenkins cleanup operation |
There was a problem hiding this comment.
I'm not knowledgeable enough regarding how those deletion happen and how to identify but it seems that we we try to achieve here is to prevent submitting an "AbortedEvent" for builds that are cleaned up.
A second approach could be to check the endTime of the job, if its "old enough" we can consider that this is just a clean up. But that approach could still submit some AbortedEvent if the clean up happens "too fast".
Not saying this approach is better but wanted to make sure we choose the best approach. Your comment mentions that "it could be a Jenkins cleanup operation", we'd rather be confident regarding that assumption before merging. Any thoughts?
There was a problem hiding this comment.
Agree that we want to get this right. The potential issue with time gating the results is it will vary wildly depending on how a user has their Jenkins config setup. They could have a limit of 10 builds for something that runs once a day (builds deleted after 10 days), or they could have a recurring job that flushes things within an hour or two. Picking a good default would be the hard part, and may end up causing confusion if some events are reported as Aborted and others are silently dropped.
The thought (possibly flawed) for this approach was that a build which had a status already assigned would have sent an event prior to this one, so its progress would have been recorded at Datadog. Builds which get deleted before reaching that stage seem like the important ones to track.
We could also add an option on whether to send these events and default it to True - it may be that some customers like that "end-to-end" visibility and some would rather clean it up.
There was a problem hiding this comment.
Have you validated that approach with a test instance of Jenkins? If so I think this is the best approach to choose and we can move forward with it.
There was a problem hiding this comment.
Yes, have confirmed it on a local test instance and verified that completed jobs didn't have Aborted events sent when they were deleted.
Next will add a test to this PR to make sure we have the functionality covered. It's a little tricky since the activity spans two existing test classes (BuildAbortedEventTest.java and DatadogBuildListenerTest.java).
| } | ||
|
|
||
| // If the build already complete, this could be a Jenkins cleanup operation | ||
| if (buildData.isCompleted()) { |
There was a problem hiding this comment.
Note: updated this clause to check for the underlying completeBuild flag instead of null. See the breakdown of what's considered complete in the Result class.
ofek
left a comment
ofek
left a comment
There was a problem hiding this comment.
Logic appears to be sound 👍
Co-authored-by: Florian Veaux <[email protected]>
mgarabed
added
changelog/Changed
3 participants
Requirements for Contributing to this repository
What does this PR do?
In cases where Jenkins is configured to only keep the latest X builds, there will be a deletion event for old builds. These get reported to Datadog as
BuildAbortedEventand can be confusing to the user.This PR checks if the deleted job has actually completed already, and if so, it will have sent an event with its results previously, so can be ignored.
Description of the Change
Alternate Designs
Possible Drawbacks
It's possible the user may want to see these events, in which case we should perhaps add a config option to allow it? That seems unlikely, but feedback is welcome.
Verification Process
Additional Notes
Release Notes
Review checklist (to be filled by reviewers)
changelog/label attached. If applicable it should have thebackward-incompatiblelabel attached.do-not-merge/label attached.kind/andseverity/labels attached at least.