Skip to content
This repository has been archived by the owner on Jan 23, 2020. It is now read-only.

Send all metrics w/o DogstatsD #169

Merged
merged 30 commits into from
Nov 27, 2019
Merged

Send all metrics w/o DogstatsD #169

merged 30 commits into from
Nov 27, 2019

Conversation

dabcoder
Copy link
Contributor

@dabcoder dabcoder commented Nov 14, 2019

  • Get rid of the DogStatsD dependency by creating a thread safe counter cache
    • Use a ConcurrentMap to make sure that increments are thread safe - prevent concurrency issue - I may have been over protective with my implementation
    • Use a ReentrantLock to make sure we don't flush and increment the counter cache at the same time (across threads).
    • Use the singleton pattern to only have one instance of the cache in the JVM (also thread safe)
    • Counter are submitted as COUNT with an interval set to 10 s (recommended interval)
  • It also adds jenkins.job.started and jenkins.scm.checkout metrics.
  • It increases test coverage!
  • Wrapping all extensions in global try catch statements so build don't fail because of the plugin - see issue discussed in Provide option to post to local dogstatsd rather than going directly to the API #34 (however, in this PR we are removing DogStatsD dependency)
  • Changed the DatadogHttpClient into another singleton - thread safe - Hence we don't have lease logic anymore from the descriptor.

Copy link
Contributor

@gzussa gzussa left a comment

Choose a reason for hiding this comment

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

Didn't check tests at this point. Just the core implementation. Good clean up 👍

@dabcoder dabcoder marked this pull request as ready for review November 18, 2019 12:06
Copy link
Contributor

@gzussa gzussa left a comment

Choose a reason for hiding this comment

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

Great. Made comment to push this further.

  • Let's manually test this to make sure it work. I have a doubt that resetting the cache before submitting metrics may be a problem.
  • Also we should go deeper testing mttr, cycleTime, leadTime metrics. Each deserve their own tests.

@gzussa gzussa requested a review from a team November 22, 2019 16:35
Copy link
Contributor

@gzussa gzussa left a comment

Choose a reason for hiding this comment

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

Approved 😆

This was referenced Nov 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants