-
Notifications
You must be signed in to change notification settings - Fork 804
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
Add support for dw-metrics 4.x #1228
Add support for dw-metrics 4.x #1228
Conversation
2e0b416
to
fa85d06
Compare
Hi @fstab Please do review this PR, this solves for Dropwizard 4.x metrics support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution 😄
<dependency> | ||
<groupId>junit</groupId> | ||
<artifactId>junit</artifactId> | ||
<version>4.13.2</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test dependencies are added in the parent already:
Lines 103 to 143 in 9a0848d
<dependency> | |
<groupId>org.junit.jupiter</groupId> | |
<artifactId>junit-jupiter-engine</artifactId> | |
<version>${junit-jupiter.version}</version> | |
<scope>test</scope> | |
</dependency> | |
<dependency> | |
<groupId>org.junit.jupiter</groupId> | |
<artifactId>junit-jupiter-params</artifactId> | |
<version>${junit-jupiter.version}</version> | |
<scope>test</scope> | |
</dependency> | |
<dependency> | |
<groupId>org.mockito</groupId> | |
<artifactId>mockito-core</artifactId> | |
<version>5.14.2</version> | |
<scope>test</scope> | |
</dependency> | |
<dependency> | |
<groupId>org.assertj</groupId> | |
<artifactId>assertj-core</artifactId> | |
<version>3.26.3</version> | |
<scope>test</scope> | |
</dependency> | |
<dependency> | |
<groupId>com.google.guava</groupId> | |
<artifactId>guava</artifactId> | |
<scope>test</scope> | |
</dependency> | |
<dependency> | |
<groupId>org.slf4j</groupId> | |
<artifactId>slf4j-simple</artifactId> | |
<version>2.0.16</version> | |
<scope>test</scope> | |
</dependency> | |
<dependency> | |
<groupId>org.junit-pioneer</groupId> | |
<artifactId>junit-pioneer</artifactId> | |
<version>2.3.0</version> | |
<scope>test</scope> | |
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, removed these.
</dependency> | ||
<dependency> | ||
<groupId>io.prometheus</groupId> | ||
<artifactId>prometheus-metrics-exposition-formats</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should only depend on prometheus-metrics-exposition-textformats
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, changed this.
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.assertj.core.data.Offset.offset; | ||
import static org.junit.Assert.assertThrows; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use assertj for all assertions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. Fixed this
|
||
// The result should look like this | ||
// | ||
// # TYPE hist summary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did the assertion not work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works, just that its not a string comparison, hence this comment is to explain why its not asserted like in other cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you test it like here:
Line 950 in e486e4d
assertThat(out).hasToString(expectedTextFormat); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think doing it that way will lead to a flaky test-suite, because the problem is in the dw metrics4 histogram. This is something that had happened earlier and that is specially why this approach was taken.
I took reference of this method from the dw5 implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the pr to use
assertThat(textFormat)
.satisfiesAnyOf(
text -> assertThat(text).isEqualTo(expected1),
text -> assertThat(text).isEqualTo(expected2));
So that it matches the exact output, and is a much simpler test.
a07277c
to
13c320c
Compare
Signed-off-by: Kinshuk Bairagi <[email protected]>
13c320c
to
529676e
Compare
@zeitlinger Have made the changes as requested, could you please have a look again? |
Signed-off-by: Kinshuk Bairagi <[email protected]>
Signed-off-by: Gregor Zeitlinger <[email protected]>
Thank you @zeitlinger Are snapshot versions published at any repository? If not when is the next planned version release? |
1.3.5 has just been released 😄 |
Adding support for
com.codahale.metrics.MetricRegistry
(Dropwizard Metrics 4.x)Completes #867