Add a gauge to measure build waiting time. by bbeck · Pull Request #81 · DataDog/jenkins-datadog-plugin

bbeck · 2017-03-13T16:11:08Z

With a dynamically sized pool of agents it's important to know if you're scaling
appropriately and where you need to add more capacity. This PR adds a metric for
each build to let you know how long it had to wait in the queue before executing.

bbeck · 2017-03-13T16:16:52Z


    tags.add("job:" + builddata.get("job"));
-    if ( (builddata.get("node") != null) && DatadogUtilities.getDatadogDescriptor().getTagNode() ) {
+    if ( (builddata.get("node") != null) && descriptor.getTagNode() != null && descriptor.getTagNode() ) {


This was throwing a NPE upon completion of every build if you were running Jenkins with the Datadog plugin installed but not configured.

The problem was the tagNode variable (in DatadogBuildListener) is null, but the getTagNode method tries to coerce it to a Boolean and fails. Another possible way to fix this could be to make getTagNode detect this case and just return false instead. I'm happy to make this change if you feel that it's better.

Thank for pointing this out! I made PR #84 to set tagNode to false by default, this will get merged soon. Could you keep this method as is and rebase once #84 is merged?

Sure, no problem. It's removed in the most recent update.

sjenriquez

@bbeck Thanks for the PR, this will be a great enhancement! I left a few comments but looks great overall.

sjenriquez · 2017-04-11T22:47:14Z


    tags.add("job:" + builddata.get("job"));
-    if ( (builddata.get("node") != null) && DatadogUtilities.getDatadogDescriptor().getTagNode() ) {
+    if ( (builddata.get("node") != null) && descriptor.getTagNode() != null && descriptor.getTagNode() ) {


Thank for pointing this out! I made PR #84 to set tagNode to false by default, this will get merged soon. Could you keep this method as is and rebase once #84 is merged?

sjenriquez · 2017-04-11T22:55:48Z

+      } catch (InterruptedException ex) {
+        logger.severe(ex.getMessage());
+      }
+


I think we should abstract this to a new method in DataUtilities.java so we can use it for future metrics as well. I was playing around with this and added this method:

/** * Builds extraTags if any are configured in the Job * * @param run - Current build * @param listener - Current listener * @return A {@link HashMap} containing the key,value pairs of tags if any. */ public static HashMap<String,String> buildExtraTags(Run run, TaskListener listener) { HashMap<String,String> extraTags = new HashMap<String, String>(); try { extraTags = DatadogUtilities.parseTagList(run, listener); } catch (IOException ex) { logger.severe(ex.getMessage()); } catch (InterruptedException ex) { logger.severe(ex.getMessage()); } return extraTags; }

Then we can just add these lines here and on line 160 as well:

HashMap<String,String> extraTags = new HashMap<String, String>(); extraTags = DatadogUtilities.buildExtraTags(run, listener);

Sure. Just pushed an update that introduces this helper like you suggested.

sjenriquez · 2017-04-14T17:43:45Z

Thanks @bbeck, this looks good to go. Can you just rebase on master to resolve the merge conflict?

bbeck · 2017-05-05T18:49:54Z

@sjenriquez Resolved the merge conflict and squashed commits. It does look however like some tests are failing possibly related to this PR -- it seems that Jenkins.getInstance() is returning null when the test is initialized? Have you seen anything like this before? These tests were passing when I first submitted the PR, so maybe something has changed since?

sjenriquez · 2017-05-25T20:59:24Z

@bbeck Sorry for the troubles here, indeed a PR was merged that broke the tests. This has been resolved and we've set up Travis-CI to avoid these issues in the future. Could you please resolve the conflicts and push one more time. Thanks!

* datadog/master: push back date fix date add release date update changelog update changelog update changelog

bbeck · 2017-07-20T17:32:10Z

@sjenriquez Sorry for the delay. I resolved conflicts, but the tests are still failing for what seems to be the same reason. Jenkins.getInstance() is returning null when called from Queue.getInstance(). I'm unsure of how to fix this -- this test used to run successfully. Any ideas?

Caused by: java.lang.NullPointerException
	at hudson.model.Queue.getInstance(Queue.java:2856)
	at org.datadog.jenkins.plugins.datadog.DatadogBuildListener.<clinit>(DatadogBuildListener.java:81)
	... 38 more

sjenriquez · 2017-07-20T18:36:45Z

Hey @bbeck, looks like we need some additional mocking. More info available here. Check out the diff, tests are ✅ for me with these changes.

 $ git diff
diff --git a/src/test/java/org/datadog/jenkins/plugins/datadog/DatadogBuildListenerTest.java b/src/test/java/org/datadog/jenkins/plugins/datadog/DatadogBuildListenerTest.java
index 7b10e5b..bff992f 100644
--- a/src/test/java/org/datadog/jenkins/plugins/datadog/DatadogBuildListenerTest.java
+++ b/src/test/java/org/datadog/jenkins/plugins/datadog/DatadogBuildListenerTest.java
@@ -8,9 +8,11 @@ import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
 import org.powermock.api.mockito.PowerMockito;
 import org.powermock.core.classloader.annotations.PrepareForTest;
 import org.powermock.modules.junit4.PowerMockRunner;
+import jenkins.model.Jenkins;

 import java.io.IOException;
 import java.util.HashMap;
@@ -22,13 +24,19 @@ import static org.mockito.Matchers.anyString;
 import static org.mockito.Mockito.*;

 @RunWith(PowerMockRunner.class)
-@PrepareForTest({DatadogHttpRequests.class, DatadogUtilities.class})
+@PrepareForTest({DatadogHttpRequests.class, DatadogUtilities.class, Jenkins.class})
 public class DatadogBuildListenerTest {

+    @Mock
+    private Jenkins jenkins;
+
     private DatadogBuildListener datadogBuildListener;

     @Before
     public void setUp() throws Exception {
+        PowerMockito.mockStatic(Jenkins.class);
+        PowerMockito.when(Jenkins.getInstance()).thenReturn(jenkins);
+
         datadogBuildListener = spy(new DatadogBuildListener());

         PowerMockito.mockStatic(DatadogUtilities.class);

sjenriquez · 2017-07-20T19:15:43Z

Thanks @bbeck !

bbeck commented Mar 13, 2017

View reviewed changes

sjenriquez mentioned this pull request Apr 6, 2017

Add a gauge to measure the size of the build queue. #82

Merged

sjenriquez suggested changes Apr 11, 2017

View reviewed changes

Add a gauge to measure build waiting time.

d1d863d

bbeck force-pushed the build-waiting-gauge branch from d9628c4 to d1d863d Compare May 5, 2017 18:46

truthbk and others added 2 commits June 21, 2017 16:27

Merge branch 'master' into build-waiting-gauge

07203ea

Merge remote-tracking branch 'datadog/master' into build-waiting-gauge

724ab8a

* datadog/master: push back date fix date add release date update changelog update changelog update changelog

Fix failing DatadogBuildListenerTest.

63d77af

sjenriquez approved these changes Jul 20, 2017

View reviewed changes

sjenriquez merged commit 839c95f into DataDog:master Jul 20, 2017

Uh oh!

Conversation

bbeck commented Mar 13, 2017

Uh oh!

bbeck Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjenriquez Apr 11, 2017

Choose a reason for hiding this comment

Uh oh!

bbeck Apr 13, 2017

Choose a reason for hiding this comment

Uh oh!

sjenriquez left a comment

Choose a reason for hiding this comment

Uh oh!

sjenriquez Apr 11, 2017

Choose a reason for hiding this comment

Uh oh!

sjenriquez Apr 11, 2017

Choose a reason for hiding this comment

Uh oh!

bbeck Apr 13, 2017

Choose a reason for hiding this comment

Uh oh!

sjenriquez commented Apr 14, 2017

Uh oh!

bbeck commented May 5, 2017

Uh oh!

sjenriquez commented May 25, 2017

Uh oh!

bbeck commented Jul 20, 2017

Uh oh!

sjenriquez commented Jul 20, 2017

Uh oh!

sjenriquez commented Jul 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bbeck Mar 13, 2017 •

edited

Loading