Add support for global tags (including regexes) by nmuesch · Pull Request #117 · DataDog/jenkins-datadog-plugin

nmuesch · 2018-06-18T15:13:39Z

Adds a new textarea in the plugin that allows for the addition for the placement of tags globally for jobs. This supports regex and using capture groups for your tags.

Ex: https://cl.ly/3f3r2h0R1R3q

Additional Notes:
I cleaned up the code a little bit and made the type of some objects the interface instead of the subclass type.

Additionally right now, we iterate and compile the list of jobs on each run. I think ultimately it would be best to create some singleton class that holds the configuration objects and to only compile the list of regexes when the form is re saved/re configured. That seems outside the scope of this PR and there doesn't appear to be a noticeable performance hit.

gzussa

This is a great addition to the plugin. Added comments - mostly related to Java syntax and common practices.

gzussa · 2018-07-16T09:38:33Z

    if ( DatadogUtilities.isJobTracked(jobName) ) {
      logger.fine("Started build!");
+
+      Boolean useJobRegex = DatadogUtilities.getDatadogDescriptor().getJobRegex();


Change Boolean to its primitive. This will avoid creating a new object. Also, do we have to check for null here?

After talking with @sjenriquez about the scope/UI complexity here we decided to move to a new text area for the tags option. In doing this I removed the need for this variable entirely. Good to know though!

gzussa · 2018-07-16T09:39:07Z


      // Collect Data
+      DatadogBuildListener.DescriptorImpl descriptor =  DatadogUtilities.getDatadogDescriptor();
+      Boolean useJobRegex = descriptor.getJobRegex();


gzussa · 2018-07-16T09:39:57Z

      }

+      // Grab jobRegex and coerse to a boolean
+      if ( formData.getString("jobRegex").equals("true") ) {


in case there is a null, you should do "true".equals(formData.getString("jobRegex"))

what about this.setJobRegex("true".equals(formData.getString("jobRegex")))?

gzussa · 2018-07-16T09:42:00Z

+     *
+     * @param jobRegex - a boolean flag on whether to use regexes instead of literals for whitelist/blacklisting jobs.
+     */
+    public void setJobRegex(final Boolean jobRegex) {


why do we need final?

gzussa · 2018-07-16T09:42:18Z

            && prop != null && prop.isEmitOnCheckout() ) {
      logger.fine("Checkout! in onCheckout()");

+      Boolean useJobRegex = DatadogUtilities.getDatadogDescriptor().getJobRegex();


gzussa · 2018-07-16T09:47:57Z

+   *  @param jobName - A string containing the name of some job
+   *  @return - A Map of values containing the key and value of each Datadog tag to apply to the metric/event
+   */
+  public static Map<String,String> getRegexJobTags(final String jobName) {


I don't think final is needed here either. final would be needed if there would be an interface defining the method signature as such. final only prevent for params to be reassigned within the method itself and only for its scope. Thus, it's only really useful when defined in interfaces.

gzussa · 2018-07-16T09:48:45Z

+            try {
+              tags.put(tagItem[0], m.group(Character.getNumericValue(tagItem[1].charAt(1))));
+            } catch(IndexOutOfBoundsException e) {
+              logger.fine(String.format("Specified a capture group that doesn't exist, not applying tag: " + Arrays.toString(tagItem) + " " + e));


Could use StringBuffer here

There should be a non trivial performance bump by changing all of the String.format to something like StringBuilder or StringBuffer. We should change all occurrences in a separate PR though as it seems a little out of scope here.

ok but then either you use format or + sign concatenation but both doesn't make sense to me.

gzussa · 2018-07-16T09:48:54Z

-    final List<String> blacklist = DatadogUtilities.joblistStringtoList( DatadogUtilities.getBlacklist() );
-
-    return blacklist.contains(jobName.toLowerCase());
+  public static boolean  isJobBlacklisted(final String jobName) {


ditto and everywhere else :)

gzussa · 2018-07-16T09:52:14Z


  private JSONObject builddata;
-  private HashMap<String,String> tags;
+  private Map<String,String> tags;


gzussa

Some more comments. Also, you should do a grep "final" and remove it everywhere (unless if it is required by an interface)

gzussa · 2018-07-17T13:29:32Z

    /**
-     * Setter function for the {@link jobRegex} global configuration,
-     * accepting a boolean value (checkbox)
+     * Getter function for the {@link globalJobTags} global configuration, containing


👍 nice! didn't even saw that. Good catch 😸

gzussa · 2018-07-17T13:32:02Z

+      this.setGlobalJobTags(formData.getString("globalJobTags"));
+
      // Grab tagNode and coerse to a boolean
      if ( formData.getString("tagNode").equals("true") ) {


in case there is a null, you should do "true".equals(formData.getString("jobRegex"))

gzussa · 2018-07-17T13:35:23Z

+            try {
+              tags.put(tagItem[0], m.group(Character.getNumericValue(tagItem[1].charAt(1))));
+            } catch(IndexOutOfBoundsException e) {
+              logger.fine(String.format("Specified a capture group that doesn't exist, not applying tag: " + Arrays.toString(tagItem) + " " + e));


ok but then either you use format or + sign concatenation but both doesn't make sense to me.

gzussa

Let's get this merged now

nmuesch · 2018-07-17T14:46:14Z

Thanks for all the feedback. I agree that there should be a followup cleaning PR for a couple of the brought up points 🙇

nmuesch added 4 commits June 16, 2018 01:42

Add regex tag support to Job Started Event and jenkins.waiting metric

21d2c38

Add regex tags to rest of metrics/events

2514f20

Safer lookup of groups, log if group doesn't exist

e2afce3

Update the mocks and fix tests

c6b4d39

nmuesch requested a review from sjenriquez June 30, 2018 18:25

nmuesch added the enhancement label Jun 30, 2018

gzussa reviewed Jul 16, 2018

View reviewed changes

nmuesch added 2 commits July 16, 2018 21:35

Address feedback and use new textarea

32fc39a

Remove unneeded final keyword

b3cf2a9

gzussa reviewed Jul 17, 2018

View reviewed changes

Address feedback

f61c35a

gzussa approved these changes Jul 17, 2018

View reviewed changes

nmuesch changed the title ~~Add support for regexes for whitelist/blacklist jobs and global tags~~ Add support for global tags (including regexes) Jul 17, 2018

nmuesch added 3 commits July 25, 2018 12:10

Fix issue when regex tags field is empty and remove extra mvn configs

3de783a

Fix if statement issue

84a3c66

Various clean ups

33694d4

sjenriquez merged commit 48a9843 into master Aug 7, 2018

sjenriquez deleted the nick/add_job_regex_tags branch August 7, 2018 18:04

Uh oh!

Conversation

nmuesch commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gzussa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzussa Jul 16, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzussa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzussa left a comment

Choose a reason for hiding this comment

Uh oh!

nmuesch commented Jul 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nmuesch commented Jun 18, 2018 •

edited

Loading

gzussa Jul 16, 2018 •

edited

Loading