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

Implement running jobs metric + update metrics code #407

Merged
merged 4 commits into from
Apr 8, 2024

Conversation

nikita-tkachenko-datadog
Copy link
Collaborator

@nikita-tkachenko-datadog nikita-tkachenko-datadog commented Apr 1, 2024

What does this PR do?

Adds new jenkins.job.currently_building metric which is a gauge of how many jobs are currently executing (the difference compared to existing "busy executors count" is that multiple executors can run parallel task from the same job).

Also some of the code that accumulates global counter metrics is refactored.

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.

@nikita-tkachenko-datadog nikita-tkachenko-datadog added changelog/Added Added features results into a minor version bump documentation Documentation related changes labels Apr 1, 2024
@nikita-tkachenko-datadog nikita-tkachenko-datadog force-pushed the nikita-tkachenko/metrics-update branch from a03b079 to e9bdcc4 Compare April 3, 2024 09:28
@nikita-tkachenko-datadog nikita-tkachenko-datadog marked this pull request as ready for review April 3, 2024 09:50
Copy link
Collaborator

@drodriguezhdez drodriguezhdez left a comment

Choose a reason for hiding this comment

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

Dropped some comments

metrics.gauge("jenkins.job.build_duration", buildDuration / 1000, hostname, tags);
metrics.gauge("jenkins.job.pause_duration", TimeUnit.MILLISECONDS.toSeconds(pauseDurationMillis), hostname, tags);
logger.fine(String.format("[%s]: Pause Duration: %s", buildData.getJobName(), toTimeString(pauseDurationMillis)));
long buildDuration = run.getDuration() - pauseDurationMillis;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
long buildDuration = run.getDuration() - pauseDurationMillis;
long buildDurationMillis = run.getDuration() - pauseDurationMillis;

At some point, probably we want to start using java.time.Duration and java.time.Instant to get rid of these xxxMillis suffixes everywhere, now that JDK 1.8 is a requirement for the plugin. (Not blocker for this PR, tho)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree about the java.time usage. Added the suffix for now :)

return;
}
logger.fine("increment counter with dogStatD client");
statsd.count(name, value, TagsUtil.convertTagsToArray(tags));
Copy link
Collaborator

@drodriguezhdez drodriguezhdez Apr 4, 2024

Choose a reason for hiding this comment

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

Suggested change
statsd.count(name, value, TagsUtil.convertTagsToArray(tags));
String[] tagsArr = (tags != null) ? TagsUtil.convertTagsToArray(tags) : EMPTY_ARRAY;
statsd.count(name, value, TagsUtil.convertTagsToArray(tagsArr));

Or even better, modify the TagsUtil.convertTagsToArray method to be resilient for null tags.
In the DatadogApiClient class, we're checking this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a null-check in TagsUtil.convertTagsToArray

metrics.gauge("jenkins.node.count", nodeCount, hostname, globalTags);
metrics.gauge("jenkins.node.offline", nodeOffline, hostname, globalTags);
metrics.gauge("jenkins.node.online", nodeOnline, hostname, globalTags);
metrics.gauge("jenkins.job.currently_building", getCurrentRunCount(computers), hostname, globalTags);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to add a test for this new metric?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added an integration test

drodriguezhdez
drodriguezhdez previously approved these changes Apr 4, 2024
@nikita-tkachenko-datadog nikita-tkachenko-datadog merged commit 2074828 into master Apr 8, 2024
16 checks passed
@nikita-tkachenko-datadog nikita-tkachenko-datadog deleted the nikita-tkachenko/metrics-update branch April 8, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/Added Added features results into a minor version bump documentation Documentation related changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants