Count metric for completed jobs #39
Conversation
|
Hey @MadsNielsen! Okay, so this PR works, but it is not the ideal way of doing this. First of all, it's not entirely accurate to use a gauge in such a way, and it'll require using rollups and things on a dashboard to make it work. Additionally, multiple jobs finishing at the same second will not be counted correctly. Let's shoot out a UDP packet to the local Dogstatsd (packaged with the datadog agent). If it's not there, the UDP packets will fall on nothing, if it is there, then the counts will be pushed up to Datadog. Let me know if I can clarify anything here for you. |
|
@JohnLZeller Sounds fine by me. I assume this is documented in your API docs so i can implement it? |
|
@MadsNielsen It's not in the API docs actually, because it's an agent feature, not a feature of the API. |
|
Hey @JohnLZeller I've started working on this, and a couple of questions popped up.
But i think i got the overall idea on how this works so i'm working on getting this done :) |
|
Hey @JohnLZeller Another question popped up: What is your thought of using one of the excellent client libraries you're linking to in the plugin? More specifically im lookin at this: https://github.com/indeedeng/java-dogstatsd-client |
|
@MadsNielsen Yeah, but let's use our own client fork, https://github.com/DataDog/java-dogstatsd-client
|
|
@MadsNielsen once you've implemented the new functionality we spoke of, would you rebase this PR from master (I merged your other changes)? Thanks :) |
|
@JohnLZeller Okay. First draft for this feature is ready. I used your client fork for this part. The configuration now includes an I found the Client reasonably ok to work with, one issue i had initially was that i just couldn't just create a new client in the So...i think i figured out how to do it properly. When the global configuration page is saved. We need to pick up eventual configuration changes and create a new client for our run listener to use. And when we use the Client in the run listener, we can ask the descriptor to give us the current configured instance... |
| client = new NonBlockingStatsDClient("jenkins.job", hp, pp); | ||
| logger.finer(String.format("Created new dogstatsdaemon client (%s:%S)!", hp, pp)); | ||
| } catch (Exception e) { | ||
| logger.log(Level.SEVERE, "Unable to crete new DogstatsClient", e); |
|
@MadsNielsen okay there is my first round of feedback. I'm gonna run some more tests on this :) |
|
@JohnLZeller Great! I tried as best to test this, i think i had a setup with like 10 jobs running at intervals pushing events to datadog. I'll wait ammending this PR until you've finished your validation inhouse so that we can finish it up in a single go. |
|
@MadsNielsen looks good, other than the nitpicks :) |
|
@JohnLZeller great! Im on vacation now and Will be back after next weekend. I'll fix it then. Is this ok for your release schedule.? |
|
@MadsNielsen we were hoping to release this a bit sooner actually. Would you mind if I rebased your commit and made the small changes? The commit will show up as a pair commit, but you'll still have attribution on the things you changed, and in the commit user. |
|
@MadsNielsen here is the final PR, with your permission I'd like to merge this #43 |
Count metric for completed jobs #39
|
Merged #43 |
|
Great you got it fixed! @JohnLZeller im perfectly fine with you fixing the issues you pointed out in your review while i am away😁 |
Here you go @JohnLZeller . This is the pull request for the 'count' metric.
The PR also includes the changes for the tags...which this requires. The relevant part to review would then be in the second commit of this PR.