DropWizard 4 [Do Not Merge] #59
Conversation
mikebell90
requested review from
rushalias,
SegFaultAX,
adamzr,
dkaukov and
strykerd
| @@ -1,6 +1,15 @@ | |||
| otj-metrics | |||
| =========== | |||
|
|
|||
| 4.0.0 | |||
There was a problem hiding this comment.
@adamzr no because your changes are 3.0.0 (incompatible api change). This has the advantage of us being able to mentally say "Oh DropWizard 4.x == otj-metrics 4.x
| This is an incompatible binary/source change, which will require users to import new versions, and a new | ||
| module (metrics-jmx) | ||
|
|
||
| In addition, a fork of metrics-spring will probably be needed |
There was a problem hiding this comment.
We should reach out to metrics-spring maintainer and suggest that he either:
a) quickly update his project
b) add us as maintainers and allow us to update his project for him
c) mark his project as abandoned in favor of a fork that we build
Otherwise, we can either give up on the library, or we can build our own fork.
There was a problem hiding this comment.
@adamzr I have a fork and a PR put up. Take a look at https://github.com/ryantenney/metrics-spring the original github. Pretty much dead eh? Look at the PRs that have waited for years.
| * (In DropWizard 3.1.3+, each reporting period, a new socket (including DNS resolution) is created, metrics are sent, and | ||
| * the socket is closed. All the previous issues occurred because of atttempts to reuse a socket) | ||
| * Hence, this class is now mostly a "dumb" delegator, with little extra logic other than | ||
| * metrics wrapping, and a restart per hour. |
There was a problem hiding this comment.
I think we should dump the hourly restart.
There was a problem hiding this comment.
It's out of an excess of caution tbh. I could be argued/cajoled into it ;)
| // Spin up new one | ||
| Graphite newGraphite = new Graphite(address.get()); | ||
| newGraphite.connect(); | ||
| //newGraphite.connect(); |
|
|
||
| // Wait for graphite to connect. | ||
| Thread.sleep(reportingPeriod.multipliedBy(2).toMillis()); | ||
| Assert.assertTrue(server.getBytesRead() > 0); | ||
| Assert.assertEquals(0, detectedConnectionFailures.getCount()); | ||
|
|
||
| server.contains("wolf created but otherwise default", "foo"); |
There was a problem hiding this comment.
I added a sort of TDD like clause soo I could debug.
|
Note #61 is a report for server 3.0. This PR will never be merged and is being kept open for @rushalias convenience for now. |
|
Closed in favor of #61 |
Can be merged (refactored) after other stuff
Required for Spring Boot 2.1
Basic Changes were simple in theory
However most of a day was spent exploring why the Reconnect tests failed.
In turns out due to dropwizard/metrics#694
that all versions of DW from 3.1.3+ (3.1.2 is currently used), including the 3.2.3 override (abtest clients) were failing, because most of the logic of GraphiteSenderWrapper is redundant and broken in a refactored GraphiteReporter that connects/closes on every reporting period. Hence much of that class was gutted. Even the hourly reconnect I suspect no longer serves a purpose.
Remaining: