Reduce number of tags for micrometer #3511
Conversation
There was a problem hiding this comment.
Approach looks good. But I think there are a couple more places like
/content/{key}
/trees//tree/{refName} (getReferenceByName)
/trees//tree/{refType}/{refName} (assign/delete, can be a "tag" as well)
/trees//tree/branch/{refName}/transplant
/trees//tree/branch/{refName}/merge
Codecov Report
@@ Coverage Diff @@
## main #3511 +/- ##
============================================
+ Coverage 86.73% 86.94% +0.20%
+ Complexity 3046 2958 -88
============================================
Files 376 369 -7
Lines 16410 16013 -397
Branches 1214 1177 -37
============================================
- Hits 14233 13922 -311
+ Misses 1731 1664 -67
+ Partials 446 427 -19
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Can we add some simple test to make sure we're getting all of the required metrics from #3510?
Something like:
@QuarkusTest
@TestProfile(value = QuarkusTestProfileInmemory.class)
public class ITTestMetrics {
@Test
void smokeTestMetrics() {
String body = RestAssured.given()
.when()
.basePath("/q/metrics")
.get()
.then()
.statusCode(200)
.extract()
.asString();
assertThat(body).contains("jvm_threads_live_threads");
// ... verify the other important metrics
}
}
I've added additional mappings; however, I am not sure about the |
It's in org.projectnessie.api.http.HttpContentApi#getContent |
|
Out of scope and maybe something for later, but is there a way to automatically generate these configs from an openapi spec? |
We are hitting this: quarkusio/quarkus#21034 so once it will be resolved, the mapping could be removed. So I am not sure if we want to create a mechanism for generating that. |
Ah! GTK! |
|
LGTM! Can you open a revert-PR that we can then just merge once a Quarkus version with the fix gets released? The PR as a "permanent reminder". |
@snazy How do we prevent someone from accidentally merging such a PR? Maybe it's better to create a follow-up ticket? |
| .asString(); | ||
|
|
||
| // then | ||
| assertThat(body).contains("jvm_threads_live_threads"); |
There was a problem hiding this comment.
seems ok for now. We'll follow up with whatever other metrics we want to verify in a separate PR, so let's merge what we have
There was a problem hiding this comment.
Just open it as a draft, nobody will/should touch it then
|
I am having some trouble reproducing the Quarkus issue mentioned above with the provided reproducer. In your case, how are you making outbound Http requests? The current support looks for usage of the Rest Client (which declares templated URIs in a way we can look for at build time). |
Before the fix in this ticket, the the although our interfaces that are defining URLs were using template parameters, for example: To reproduce it in our project, you can run this test: https://github.com/projectnessie/nessie/blob/cd24dcfc77fd1495e646dbaef521acc7e94c412f/servers/quarkus-server/src/test/java/org/projectnessie/server/ITRestApiDynamo.java Please let me know if you have more questions. |
|
Thank you! This is server side, not client side, right? (received requests, rather than outbound)? |
yes, server-side. |
|
I am also facing a similar issue of higher number of URI tags. Since I am planning to follow the same path that you guys have followed, I have a question regarding having multiple path parameters. In our case, the application has three path parameters. Let's say: Param2 has limited set of values and will never lead to high cardinality situations. |
Using the mechanism described in: https://access.redhat.com/documentation/en-us/red_hat_build_of_quarkus/1.11/html/collecting_metrics_in_your_quarkus_applications/con-http-metrics_quarkus-collecting-metrics
It reduces the number of tags reported by the metrics registry, by using
{path_parameters}.After the fix, tags look like this:
so the number of tags is substantially reduced and the warning is no longer emitted.