Add new node status metrics by sarah-witt · Pull Request #71 · jenkinsci/datadog-plugin

sarah-witt · 2020-06-09T19:30:01Z

Requirements for Contributing to this repository

Fill out the template below. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
The pull request must only fix one issue at the time.
The pull request must update the test suite to demonstrate the changed functionality.
After you create the pull request, all status checks must be pass before a maintainer reviews your contribution. For more details, please see CONTRIBUTING.

What does this PR do?

Adds new node status metrics that are tagged by node, so that we can determine which nodes are offline/online, and when.
These metrics are submitted as a 1 or 0, but when summed across nodes/labels, they can also show total number of nodes, etc.

Description of the Change

Alternate Designs

Possible Drawbacks

Verification Process

Additional Notes

Release Notes

Review checklist (to be filled by reviewers)

Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
PR title must be written as a CHANGELOG entry (see why)
Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
PR must have one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
PR should not have do-not-merge/ label attached.
If Applicable, issue must have kind/ and severity/ labels attached at least.

sarah-witt · 2020-06-15T19:27:03Z

@@ -0,0 +1,15 @@
+package org.datadog.jenkins.plugins.datadog.publishers;


An alternative way to stub the client would be to use PowerMock as in: #74, which means we wouldn't need this file and to add a wrapper getDatadogClient() method.

sarah-witt · 2020-06-15T19:29:59Z

+        expectedTags[1] = "node_label:master";
+        expectedTags[2] = "jenkins_url:" + url;
+
+        // The CI sets a hostname but we cannot set a hostname locally: https://javadoc.jenkins-ci.org/hudson/model/Computer.html#getHostName--


This case was tricky- locally computer.getHostname() does not return anything, but on the CI it does. I couldn't find a good way to update this, see https://javadoc.jenkins-ci.org/hudson/model/Computer.html#getHostName--

ChristineTChen

I'm wondering if just 1 metric would appropriately represent the status? For example, would it make more sense if we submit jenkins.node_status.online as 1 when it's online and 0 when it's not vs submitting jenkins.node_status.offline with the opposite value?

The number of offline nodes can also be calculated with jenkins.node_status.count - jenkins.node_status.online

ChristineTChen · 2020-06-15T20:23:10Z

                nodeCount++;
+                boolean online = true;
                if (computer.isOffline()) {
+                    online = false;


Would it make sense to submit the metric here?

Suggested change

online = false;

client.gauge("jenkins.node_status.offline", isOffline, hostname, tags);

Hm that is true- both the offline and online metrics aren't needed. These metrics were based off of the jenkins.node.* metrics though, and there is both a jenkins.node.offline and a jenkins.node.online, so it would be inconsistent if we submitted only one of them for node_status. It's definitely worth considering though.

jenkins.node.offline and jenkins.node.online are total counts vs standalone status. It does work to immediately query a metric for a node to see its exact status. I was thinking in terms of metrics like a service check, we only use can_connect to determine 🟢 or 🔴

That's true! It could even be named something different like jenkins.node_status.up or something like that
cc @FlorianVeaux

I think this is a great idea, this will work exactly the same way as the datadog.agent.up metric.
So 👍

Updated! Thanks for the suggestion @ChristineTChen !

FlorianVeaux · 2020-06-16T07:49:45Z

+        expectedTags[1] = "node_label:master";
+        expectedTags[2] = "jenkins_url:" + url;
+
+        // The CI sets a hostname but we cannot set a hostname locally: https://javadoc.jenkins-ci.org/hudson/model/Computer.html#getHostName--


FlorianVeaux · 2020-06-16T08:43:12Z

            globalTags = TagsUtil.addTagToTags(globalTags, "jenkins_url", DatadogUtilities.getJenkinsUrl());
            for (Computer computer : computers) {
+                Map<String, Set<String>> tags = TagsUtil.merge(
+                        DatadogUtilities.getComputerTags(computer), globalTags);


Can we quickly benchmark this method ? Especially the call to computer.getHostname. According to docs this method can be slow as the master will try various endpoints.

At the moment this method is only used upon modification of a computer status, now it will be run periodically. Is there any layer of caching happening?

Yeah, true. I can look into it. I'm not sure if the hostname tags are needed/helpful for these metrics so it might be worth using a new method that doesn't calculate hostname.

One thing to note is I'm not changing the amount of times getComputerTags is called, this line is just moving it up in the loop: https://github.com/jenkinsci/datadog-plugin/pull/71/files#diff-e231a7baaf7bf9e40bbcfcd53c65b47cL95

Oh that's correct, thanks I missed that 👍

The method is pretty time consuming, though (locally running my new tests it takes 22.465s). It's definitely something to keep in mind when thinking of improvements to clients.

ChristineTChen

LGTM! Just a minor comment to simplify the logic :)

ChristineTChen · 2020-06-17T13:46:21Z

+                Map<String, Set<String>> tags = TagsUtil.merge(
+                        DatadogUtilities.getComputerTags(computer), globalTags);
                nodeCount++;
+                boolean online = true;


any reason to keep this variable? what if the metric is submitted during the online/offline checks?

if (computer.isOffline()) { nodeOffline++; client.gauge("jenkins.node_status.up", 0, hostname, tags); } if (computer.isOnline()) { nodeOnline++; client.gauge("jenkins.node_status.up", 1, hostname, tags); }

Sure, that way is simpler now that there's only one metric!

…into sarah/add-more-tags-nodes

ChristineTChen

Thanks!

* Add computer tags * Add node tags * Add test for hostname * correct node status * Change package * Remove multiple variables * Add more test cases * Fix formatting * Add link to comment * Add comment to new assertion * Change to node status up * Fix typo * Remove loop in tests: * Add computer tag * Remove tags * Use new client factory testing * Remove spaces * Update comment * Fix space * Add better description

sarah-witt added 4 commits June 9, 2020 15:26

Add computer tags

efc753c

Add node tags

086e5e9

Add test for hostname

3686645

correct node status

f3dd4a8

FlorianVeaux reviewed Jun 15, 2020

View reviewed changes

sarah-witt added 3 commits June 15, 2020 09:44

Change package

fd0e421

Remove multiple variables

ebb41da

Add more test cases

548f726

github-actions Bot added the documentation Documentation related changes label Jun 15, 2020

sarah-witt added 3 commits June 15, 2020 15:04

Fix formatting

09db197

Add link to comment

c557891

Add comment to new assertion

ce878aa

sarah-witt changed the title ~~Add computer tags to node metrics~~ Add new node status metrics Jun 15, 2020

sarah-witt commented Jun 15, 2020

View reviewed changes

sarah-witt marked this pull request as ready for review June 15, 2020 19:30

sarah-witt added changelog/Added Added features results into a minor version bump and removed changelog/Added Added features results into a minor version bump labels Jun 15, 2020

ChristineTChen reviewed Jun 15, 2020

View reviewed changes

sarah-witt added 2 commits June 16, 2020 10:58

Change to node status up

dc04b93

Fix typo

5afeddb

FlorianVeaux reviewed Jun 16, 2020

View reviewed changes

sarah-witt added 3 commits June 16, 2020 14:24

Remove loop in tests:

d6fb620

Add computer tag

57e2a2d

Remove tags

0d3d005

FlorianVeaux previously approved these changes Jun 17, 2020

View reviewed changes

ChristineTChen previously approved these changes Jun 17, 2020

View reviewed changes

sarah-witt added 2 commits June 17, 2020 12:51

Merge branch 'master' of https://github.com/jenkinsci/datadog-plugin …

403915b

…into sarah/add-more-tags-nodes

Use new client factory testing

542cc3d

sarah-witt dismissed ChristineTChen’s stale review via 542cc3d June 17, 2020 17:18

sarah-witt dismissed FlorianVeaux’s stale review via 542cc3d June 17, 2020 17:18

Remove spaces

f79fbb4

sarah-witt added the changelog/Added Added features results into a minor version bump label Jun 17, 2020

sarah-witt added 3 commits June 17, 2020 13:29

Update comment

98ae0ea

Fix space

7f1d983

Add better description

7a132a3

ChristineTChen approved these changes Jun 17, 2020

View reviewed changes

FlorianVeaux approved these changes Jun 22, 2020

View reviewed changes

sarah-witt merged commit f092e48 into master Jun 22, 2020

sarah-witt deleted the sarah/add-more-tags-nodes branch June 22, 2020 14:04

		@@ -0,0 +1,15 @@
		package org.datadog.jenkins.plugins.datadog.publishers;

	online = false;
	client.gauge("jenkins.node_status.offline", isOffline, hostname, tags);

Uh oh!

Conversation

sarah-witt commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Requirements for Contributing to this repository

What does this PR do?

Description of the Change

Alternate Designs

Possible Drawbacks

Verification Process

Additional Notes

Release Notes

Review checklist (to be filled by reviewers)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

ChristineTChen left a comment • 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!

ChristineTChen Jun 15, 2020 • 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarah-witt Jun 16, 2020 • 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!

ChristineTChen 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!

ChristineTChen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sarah-witt commented Jun 9, 2020 •

edited

Loading

ChristineTChen left a comment •

edited

Loading

ChristineTChen Jun 15, 2020 •

edited

Loading

sarah-witt Jun 16, 2020 •

edited

Loading