MGDSTRM-3724 Add the status code to the keycloak_request_duration_bucket metric #108

CathalOConnorRH · 2021-07-26T13:37:29Z

Motivation

https://issues.redhat.com/browse/MGDSTRM-3724
https://issues.redhat.com/browse/MGDSTRM-4318

What

Add status code and uri to the keycloak_request_duration_bucket metric

Why

This will allow filtering on specific statuses
Currently no status are returned in the metric
keycloak_request_duration_bucket{method="POST",resource="realms,realms/master/protocol/openid-connect",le="50.0",} 0.0 keycloak_request_duration_bucket{method="POST",resource="realms,realms/master/protocol/openid-connect",le="100.0",} 0.0 keycloak_request_duration_bucket{method="POST",resource="realms,realms/master/protocol/openid-connect",le="250.0",} 0.0 keycloak_request_duration_bucket{method="POST",resource="realms,realms/master/protocol/openid-connect",le="500.0",} 0.0 keycloak_request_duration_bucket{method="POST",resource="realms,realms/master/protocol/openid-connect",le="1000.0",} 1.0 keycloak_request_duration_bucket{method="POST",resource="realms,realms/master/protocol/openid-connect",le="2000.0",} 1.0 keycloak_request_duration_bucket{method="POST",resource="realms,realms/master/protocol/openid-connect",le="10000.0",} 1.0 keycloak_request_duration_bucket{method="POST",resource="realms,realms/master/protocol/openid-connect",le="30000.0",} 1.0 keycloak_request_duration_bucket{method="POST",resource="realms,realms/master/protocol/openid-connect",le="+Inf",} 1.0 keycloak_request_duration_count{method="POST",resource="realms,realms/master/protocol/openid-connect",} 1.0 keycloak_request_duration_sum{method="POST",resource="realms,realms/master/protocol/openid-connect",} 625.0

How

added status code and uri to the to the bucket output.

Verification Steps

Option 1

Build the code
Copy the artifact into $KC/standalone/deployments
Make sure the metrics logger is turned on
Play with Keycloak a bit
Look at the following url: "$KC/auth/realms/master/metrics"

Option 2

update keycloak cr to use the jar built and released from my fork
https://github.com/CathalOConnorRH/keycloak-metrics-spi/releases/tag/v2.4.1-beta

Metrics should look similar to
keycloak_request_duration_bucket{code="200",method="POST",resource="realms,realms/master/protocol/openid-connect",uri="realms/master/protocol/openid-connect/token",le="50.0",} 0.0 keycloak_request_duration_bucket{code="200",method="POST",resource="realms,realms/master/protocol/openid-connect",uri="realms/master/protocol/openid-connect/token",le="100.0",} 0.0 keycloak_request_duration_bucket{code="200",method="POST",resource="realms,realms/master/protocol/openid-connect",uri="realms/master/protocol/openid-connect/token",le="250.0",} 1.0 keycloak_request_duration_bucket{code="200",method="POST",resource="realms,realms/master/protocol/openid-connect",uri="realms/master/protocol/openid-connect/token",le="500.0",} 1.0 keycloak_request_duration_bucket{code="200",method="POST",resource="realms,realms/master/protocol/openid-connect",uri="realms/master/protocol/openid-connect/token",le="1000.0",} 1.0 keycloak_request_duration_bucket{code="200",method="POST",resource="realms,realms/master/protocol/openid-connect",uri="realms/master/protocol/openid-connect/token",le="2000.0",} 1.0 keycloak_request_duration_bucket{code="200",method="POST",resource="realms,realms/master/protocol/openid-connect",uri="realms/master/protocol/openid-connect/token",le="10000.0",} 1.0 keycloak_request_duration_bucket{code="200",method="POST",resource="realms,realms/master/protocol/openid-connect",uri="realms/master/protocol/openid-connect/token",le="30000.0",} 1.0 keycloak_request_duration_bucket{code="200",method="POST",resource="realms,realms/master/protocol/openid-connect",uri="realms/master/protocol/openid-connect/token",le="+Inf",} 1.0 keycloak_request_duration_count{code="200",method="POST",resource="realms,realms/master/protocol/openid-connect",uri="realms/master/protocol/openid-connect/token",} 1.0

Checklist:

Code has been tested locally by PR requester
Changes have been successfully verified by another team member

Progress

Finished task
TODO

Additional Notes

PS.: Add images and/or .gifs to illustrate what was changed if this pull request modifies the appearance/output of something presented to the users.

Rajagopalan-Ranganathan · 2021-08-06T14:00:15Z

src/test/java/org/jboss/aerogear/keycloak/metrics/PrometheusExporterTest.java

@@ -265,28 +265,28 @@ public void shouldCorrectlyRecordGenericAdminEvents() throws IOException {

 @Test
 public void shouldCorrectlyRecordResponseDurations() throws IOException {
- PrometheusExporter.instance().recordRequestDuration(5, "GET", "admin,admin/serverinfo");
+ PrometheusExporter.instance().recordRequestDuration(200, 5, "GET", "admin,admin/serverinfo", "auth/realm");


A doubt : should the URI be only "auth/realm" or it can be other paths as well? (since all the tests use this , wanted to understand)

the auth/realm URI is just an example as part of the test, Any URI path can be used

ok got it. thanks!

I think it would be good to wait for @pb82. As I understand it just using the RAW URI caused issues in the past especially when there were unique ids in the path as it created too many timeseries. Ideally these URIs would be translated into something like realms/{realm}/protocol/{protocol}/token however that could be a lot of effort so perhaps better to choose some important APIs and just covers those initially.

The openid-connect ones would be important ones along with service account mgmt

@CathalOConnorRH the raw URL should not be added as a label, this is what caused #64 in the past

@CathalOConnorRH but it looks like you're already using the resource extractor instead of the raw URL. That's better, but keep in mind that every combination of labels leads to a new exported histogram (and that means one additional mertric per bucket plus sum and total). So if we keep adding labels to histograms the amount of metrics will grow (and has grown to huge numbers in the past).

Rajagopalan-Ranganathan · 2021-08-06T14:57:48Z

/lgtm

akoserwal · 2021-08-06T15:43:07Z

@pb82 Would like to take a look as well?

akoserwal · 2021-08-06T15:45:37Z

LGTM

pb82 · 2021-08-09T07:58:10Z

@CathalOConnorRH if it is absolutely required to add those labels to the histogram, can we make that opt-in behind a flag?

CathalOConnorRH · 2021-08-09T11:54:11Z

@pb82 I've updated the ResourceExtractor to include a URI_METRICS_ENABLED boolean and a check for /token URI.
an example of the metrics output is attached

Let me know what you think

KeycloakMetrics.txt

CathalOConnorRH · 2021-08-11T14:27:25Z

Hey @pb82 Have you had any thoughts on the above changes ?

pb82 · 2021-08-17T08:13:31Z

thanks @CathalOConnorRH I'll give it a final try and then merge

Rajagopalan-Ranganathan · 2021-08-23T22:58:12Z

@pb82 did you find time to try it out? Just curious about the status. Thanks!

maleck13 · 2021-08-30T08:33:57Z

src/main/java/org/jboss/aerogear/keycloak/metrics/ResourceExtractor.java

+ * @return The resource uri.
+ */
+ static String getURI(UriInfo uriInfo) {
+ if (URI_METRICS_ENABLED) {


does it make sense here to replace certain parts of the URI with a place holder to reduce uniqueness? For example replace /realm/master with /realm/{realm} for example. Looking for ways to reduce uniqueness while still providing the needed insight

That's possible, We could do another flag to enable more detailed realm output if needed ?

@CathalOConnorRH if we add that, we can also add the change were we remove the labels completely if URI_METRICS_ENABLED is not set (at the moment they are put on all metrics with an empty value).

agreed, I'll work on both of those changes today

…etric

Rajagopalan-Ranganathan · 2021-08-30T21:12:00Z

src/main/java/org/jboss/aerogear/keycloak/metrics/MetricsFilter.java

@@ -23,6 +23,8 @@
 private static final String METRICS_REQUEST_TIMESTAMP = "metrics.requestTimestamp";
 private static final MetricsFilter INSTANCE = new MetricsFilter();

+ private static final boolean URI_METRICS_ENABLED = Boolean.getBoolean("URI_METRICS_ENABLED");


Not a JAva guy, so trying to understand would it default to false, in case the variable is not defined?

according to the docs:

Returns true if and only if the system property named by the argument exists and is equal to the string "true". (Beginning with version 1.0.2 of the JavaTM platform, the test of this string is case insensitive.) A system property is accessible through getProperty, a method defined by the System class. If there is no property with the specified name, or if the specified name is empty or null, then false is returned.

Yeah, if the value is set and set to true it will return true, otherwise it returns false
https://www.tutorialspoint.com/java/lang/boolean_getboolean.htm

CathalOConnorRH · 2021-08-31T08:36:51Z

@pb82 I've implemented the detailed output and removed the uri when it's not enabled.
Some sample output of each of the three scenarios are attached.
Let me know what you think
URIEnabledURIDetailed.txt
URIDisabled.txt
URIEnabled.txt

Rajagopalan-Ranganathan · 2021-09-03T14:33:45Z

@pb82 We need this change in MAS SSO soon, to build some metrics - dashboards and also to investigate and get precise information regarding our load (which end points are used most). Can you take a look and close this at the earliest?

Mgdstrm 3724

CathalOConnorRH force-pushed the MGDSTRM-3724 branch 3 times, most recently from ced6587 to c982927 Compare August 6, 2021 11:24

Rajagopalan-Ranganathan reviewed Aug 6, 2021

View reviewed changes

CathalOConnorRH force-pushed the MGDSTRM-3724 branch from c982927 to 81c9b6b Compare August 9, 2021 11:52

pb82 approved these changes Aug 17, 2021

View reviewed changes

maleck13 reviewed Aug 30, 2021

View reviewed changes

Add the status code and uri to the keycloak_request_duration_bucket m…

20fe4a6

…etric

CathalOConnorRH force-pushed the MGDSTRM-3724 branch from 81c9b6b to 20fe4a6 Compare August 30, 2021 21:06

Rajagopalan-Ranganathan reviewed Aug 30, 2021

View reviewed changes

Georg Modzelewski and others added 3 commits September 5, 2021 13:42

Upgrade gradle wrapper, use implementation instead of compile

3d3eaee

fix: use env var instead of system property, fix tests

f921037

Merge pull request #1 from pb82/MGDSTRM-3724

7b60b74

Mgdstrm 3724

pb82 merged commit 4dd6b7b into aerogear:master Sep 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MGDSTRM-3724 Add the status code to the keycloak_request_duration_bucket metric #108

MGDSTRM-3724 Add the status code to the keycloak_request_duration_bucket metric #108

CathalOConnorRH commented Jul 26, 2021 •

edited

Loading

Rajagopalan-Ranganathan Aug 6, 2021

CathalOConnorRH Aug 6, 2021

Rajagopalan-Ranganathan Aug 6, 2021

maleck13 Aug 9, 2021 •

edited

Loading

maleck13 Aug 9, 2021

pb82 Aug 9, 2021

pb82 Aug 9, 2021

Rajagopalan-Ranganathan commented Aug 6, 2021

akoserwal commented Aug 6, 2021

akoserwal commented Aug 6, 2021

pb82 commented Aug 9, 2021

CathalOConnorRH commented Aug 9, 2021

CathalOConnorRH commented Aug 11, 2021

pb82 commented Aug 17, 2021

Rajagopalan-Ranganathan commented Aug 23, 2021

maleck13 Aug 30, 2021

CathalOConnorRH Aug 30, 2021

pb82 Aug 30, 2021

CathalOConnorRH Aug 30, 2021

Rajagopalan-Ranganathan Aug 30, 2021

pb82 Aug 31, 2021

CathalOConnorRH Aug 31, 2021

CathalOConnorRH commented Aug 31, 2021

Rajagopalan-Ranganathan commented Sep 3, 2021

MGDSTRM-3724 Add the status code to the keycloak_request_duration_bucket metric #108

MGDSTRM-3724 Add the status code to the keycloak_request_duration_bucket metric #108

Conversation

CathalOConnorRH commented Jul 26, 2021 • edited Loading

Motivation

What

Why

How

Verification Steps

Option 1

Option 2

Checklist:

Progress

Additional Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maleck13 Aug 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rajagopalan-Ranganathan commented Aug 6, 2021

akoserwal commented Aug 6, 2021

akoserwal commented Aug 6, 2021

pb82 commented Aug 9, 2021

CathalOConnorRH commented Aug 9, 2021

CathalOConnorRH commented Aug 11, 2021

pb82 commented Aug 17, 2021

Rajagopalan-Ranganathan commented Aug 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CathalOConnorRH commented Aug 31, 2021

Rajagopalan-Ranganathan commented Sep 3, 2021

CathalOConnorRH commented Jul 26, 2021 •

edited

Loading

maleck13 Aug 9, 2021 •

edited

Loading