API usage monitoring filter improvements (#950)

* feat(api-usage-monitoring-filter): use {no-match} for not matched paths (instead of <unknown>) (#949) Signed-off-by: Maxim Tschumak <maxim.tschumak@gmail.com> * feat(api-usage-monitoring-filter): fail (not create a filter) on invalid config (#949) Signed-off-by: Maxim Tschumak <maxim.tschumak@gmail.com> * feat(api-usage-monitoring-filter): path parameters in curly braces (#949) Signed-off-by: Maxim Tschumak <maxim.tschumak@gmail.com> * feat(api-usage-monitoring-filter): realms via global config and client pattern on filter level (incompatible interface change) (#949) Signed-off-by: Maxim Tschumak <maxim.tschumak@gmail.com> * feat(api-usage-monitoring-filter): improvements proposed by @tkrop (#949) Signed-off-by: Maxim Tschumak <maxim.tschumak@gmail.com> * feat(api-usage-monitoring-filter): use regex for realms to track (#949) Signed-off-by: Maxim Tschumak <maxim.tschumak@gmail.com>
zalando · Feb 13, 2019 · adbe416 · adbe416
1 parent 3b8f034
commit adbe416
Show file tree

Hide file tree

Showing 11 changed files with 286 additions and 252 deletions.
diff --git a/cmd/skipper/main.go b/cmd/skipper/main.go
@@ -73,6 +73,7 @@ const (
 	defaultApiUsageMonitoringRealmKeys                    = ""
 	defaultApiUsageMonitoringClientKeys                   = "sub"
 	defaultApiUsageMonitoringDefaultClientTrackingPattern = ""
+	defaultApiUsageMonitoringRealmsTrackingPattern        = "services"
 
 	// generic:
 	addressUsage                         = "network address that skipper should listen on"
@@ -168,7 +169,8 @@ const (
 	apiUsageMonitoringEnableUsage                       = "enables the apiUsageMonitoring filter"
 	apiUsageMonitoringRealmKeysUsage                    = "name of the property in the JWT payload that contains the authority realm"
 	apiUsageMonitoringClientKeysUsage                   = "comma separated list of names of the properties in the JWT body that contains the client ID"
-	apiUsageMonitoringDefaultClientTrackingPatternUsage = "regular expression to default to when API usage monitoring filter configuration does not provide `client_tracking_pattern`"
+	apiUsageMonitoringDefaultClientTrackingPatternUsage = "*Deprecated*: set `client_tracking_pattern` directly on filter"
+	apiUsageMonitoringRealmsTrackingPatternUsage        = "regular expression used for matching monitored realms (defaults is 'services')"
 
 	// connections, timeouts:
 	idleConnsPerHostUsage             = "maximum idle connections per backend host"
@@ -310,6 +312,7 @@ var (
 	apiUsageMonitoringRealmKeys                    string
 	apiUsageMonitoringClientKeys                   string
 	apiUsageMonitoringDefaultClientTrackingPattern string
+	apiUsageMonitoringRealmsTrackingPattern        string
 
 	// connections, timeouts:
 	idleConnsPerHost             int
@@ -449,6 +452,7 @@ func init() {
 	flag.StringVar(&apiUsageMonitoringRealmKeys, "api-usage-monitoring-realm-keys", defaultApiUsageMonitoringRealmKeys, apiUsageMonitoringRealmKeysUsage)
 	flag.StringVar(&apiUsageMonitoringClientKeys, "api-usage-monitoring-client-keys", defaultApiUsageMonitoringClientKeys, apiUsageMonitoringClientKeysUsage)
 	flag.StringVar(&apiUsageMonitoringDefaultClientTrackingPattern, "api-usage-monitoring-default-client-tracking-pattern", defaultApiUsageMonitoringDefaultClientTrackingPattern, apiUsageMonitoringDefaultClientTrackingPatternUsage)
+	flag.StringVar(&apiUsageMonitoringRealmsTrackingPattern, "api-usage-monitoring-realms-tracking-pattern", defaultApiUsageMonitoringRealmsTrackingPattern, apiUsageMonitoringRealmsTrackingPatternUsage)
 
 	// connections, timeouts:
 	flag.IntVar(&idleConnsPerHost, "idle-conns-num", proxy.DefaultIdleConnsPerHost, idleConnsPerHostUsage)
@@ -650,9 +654,10 @@ func main() {
 
 		// API Monitoring:
 		ApiUsageMonitoringEnable:                       apiUsageMonitoringEnable,
-		ApiUsageMonitoringRealmKey:                     apiUsageMonitoringRealmKeys,
-		ApiUsageMonitoringClientIdKeyName:              apiUsageMonitoringClientKeys,
+		ApiUsageMonitoringRealmKeys:                    apiUsageMonitoringRealmKeys,
+		ApiUsageMonitoringClientKeys:                   apiUsageMonitoringClientKeys,
 		ApiUsageMonitoringDefaultClientTrackingPattern: apiUsageMonitoringDefaultClientTrackingPattern,
+		ApiUsageMonitoringRealmsTrackingPattern:        apiUsageMonitoringRealmsTrackingPattern,
 
 		// Auth:
 		OAuthUrl:                       oauthURL,

diff --git a/docs/reference/filters.md b/docs/reference/filters.md
@@ -1050,15 +1050,15 @@ For the client based metrics, additional flags need to be specified.
 |--------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
 | `api-usage-monitoring-realm-keys`                      | Name of the property in the JWT JSON body that contains the name of the _realm_.                                                                                                                                         |
 | `api-usage-monitoring-client-keys`                     | Name of the property in the JWT JSON body that contains the name of the _client_.                                                                                                                                        |
-| `api-usage-monitoring-default-client-tracking-pattern` | Optional. Default is empty. Allows to deploy Skipper with a default client tracking pattern. When `apiUsageMonitoring` configuration does not specify one, this default is used instead of disabling the client metrics. |
+| `api-usage-monitoring-realms-tracking-pattern`         | RegEx of _realms_ to be monitored. Defaults to 'services'.                                                                                                                                                                |
 
 NOTE: Make sure to activate the metrics flavour proper to your environment using the `metrics-flavour`
 flag in order to get those metrics.
 
 Example:
 
 ```bash
-skipper -metrics-flavour prometheus -enable-api-usage-monitoring -api-usage-monitoring-realm-keys="realm" -api-usage-monitoring-client-keys="managed-id"
+skipper -metrics-flavour prometheus -enable-api-usage-monitoring -api-usage-monitoring-realm-keys="realm" -api-usage-monitoring-client-keys="managed-id" api-usage-monitoring-realms-tracking-pattern="services,users"
 ```
 
 The structure of the metrics is all of those elements, separated by `.` dots:
@@ -1085,7 +1085,7 @@ Example:
 ```
                                                                              + Realm
                                                                              |
-apiUsageMonitoring.custom.orders-backend.orders-api.GET.foo/orders/:order-id.*.*.http_count
+apiUsageMonitoring.custom.orders-backend.orders-api.GET.foo/orders/{order-id}.*.*.http_count
                                                                                | |
                                                                                | + Metric Name
                                                                                + Client
@@ -1155,6 +1155,7 @@ api-usage-monitoring-configuration:
     path_templates:
       description: Endpoints to be monitored.
       type: array
+      minLength: 1
       items:
         type: string
         description: >
@@ -1163,23 +1164,19 @@ api-usage-monitoring-configuration:
         example: /orders/{order-id}
     client_tracking_pattern:
         description: >
-            The pattern that the combination `realm.client` must match in order for the client
-            based metrics to be tracked, in form of a regular expression.
+            The pattern that matches client id in form of a regular expression.
 
-            By default (if undefined), it is set to `services\\..*`.
+            By default (if undefined), it is set to `.*`.
 
             An empty string disables the client metrics completely.
-
-            IMPORTANT: Avoid patterns that would match too many different values like `.*` or `users\\..*`. Too
-            many different metric keys would badly affect the performances of the metric systems (e.g.: Prometheus).
         type: string
         examples:
             all_services:
-                summary: All services are tracked (clients of the realm `services`).
-                value: "services\\..*"
+                summary: All services are tracked (for all activated realms).
+                value: ".*"
             just_some_services:
-                summary: Only services `orders` and `shipment` are tracked.
-                value: "services\\.(orders|shipment)"
+                summary: Only services `orders-service` and `shipment-service` are tracked.
+                value: "(orders\-service|shipment\-service)"
 ```
 
 Configuration Example:
@@ -1194,7 +1191,7 @@ apiUsageMonitoring(`
             "foo/orders/:order-id",
             "foo/orders/:order-id/order_item/{order-item-id}"
         ],
-        "client_tracking_pattern": "users\\.(joe|sabine)"
+        "client_tracking_pattern": "(shipping\-service|payment\-service)"
     }`,`{
         "application_id": "my-app",
         "api_id": "customers-api",
@@ -1209,13 +1206,13 @@ apiUsageMonitoring(`
 Based on the previous configuration, here is an example of a counter metric.
 
 ```
-apiUsageMonitoring.custom.my-app.orders-api.GET.foo/orders/:order-id.*.*.http_count
+apiUsageMonitoring.custom.my-app.orders-api.GET.foo/orders/{order-id}.*.*.http_count
 ```
 
 Here is the _Prometheus_ query to obtain it.
 
 ```
-sum(rate(skipper_custom_total{key="apiUsageMonitoring.custom.my-app.orders-api.GET.foo/orders/:order-id.*.*.http_count"}[60s])) by (key)
+sum(rate(skipper_custom_total{key="apiUsageMonitoring.custom.my-app.orders-api.GET.foo/orders/{order-id}.*.*.http_count"}[60s])) by (key)
 ```
 
 Here is an example of a histogram metric.
@@ -1230,17 +1227,13 @@ Here is the _Prometheus_ query to obtain it.
 histogram_quantile(0.5, sum(rate(skipper_custom_duration_seconds_bucket{key="apiUsageMonitoring.custom.my-app.orders-api.POST.foo/orders.*.*.latency"}[60s])) by (le, key))
 ```
 
-NOTE: Non configured paths will be tracked with `<unknown>` application ID, API ID
+NOTE: Non configured paths will be tracked with `{unknown}` application ID, API ID
 and path template.
 
-```
-apiUsageMonitoring.custom.<unknown>.<unknown>.GET.<unknown>.*.*.http_count
-```
-
 However, if all `application_id`s of your configuration refer to the same application,
 the filter assume that also non configured paths will be directed to this application.
 E.g.:
 
 ```
-apiUsageMonitoring.custom.my-app.<unknown>.GET.<unknown>.*.*.http_count
-```
+apiUsageMonitoring.custom.my-app.{unknown}.GET.{no-match}.*.*.http_count
+```
diff --git a/filters/apiusagemonitoring/doc.go b/filters/apiusagemonitoring/doc.go
@@ -35,8 +35,8 @@ Command line example for executing locally:
 		-routes-file "$HOME/temp/test.eskip" \
 		-enable-api-usage-monitoring \
 		-api-usage-monitoring-realm-keys="https://identity.zalando.com/realm" \
-		-api-usage-monitoring-client-keys="https://identity.zalando.com/managed-id" \
-		-api-usage-monitoring-default-client-tracking-pattern="services\\..*" \
+		-api-usage-monitoring-client-keys="https://identity.zalando.com/managed-id,sub" \
+		-api-usage-monitoring-realmsTrackingPattern-tracking-pattern="services" \
 		-metrics-flavour prometheus \
 		-histogram-metric-buckets=".01,.025,.05,.075,.1,.2,.3,.4,.5,.75,1,2,3,4,5,7,10,15,20,30,60,120,300,600" \
 		-application-log-level=DEBUG

diff --git a/filters/apiusagemonitoring/filter.go b/filters/apiusagemonitoring/filter.go
@@ -104,45 +104,40 @@ func (f *apiUsageMonitoringFilter) getClientMetricsNames(realmClientKey string,
 	return prefixes
 }
 
-// getRealmClientKey generates the proper <Realm>.<Client ID> part of the
-// client metrics name.
-func (f *apiUsageMonitoringFilter) getRealmClientKey(r *http.Request, path *pathInfo) string {
-	const (
-		unknownRealmClient           = unknownElementPlaceholder + "." + unknownElementPlaceholder
-		unknownClientAfterKnownRealm = "." + unknownElementPlaceholder
-	)
+const unknownUnknown = unknownPlaceholder + "." + unknownPlaceholder
 
-	// no JWT ==>  <unknown>.<unknown>
+// getRealmClientKey generates the proper <realm>.<client> part of the client metrics name.
+func (f *apiUsageMonitoringFilter) getRealmClientKey(r *http.Request, path *pathInfo) string {
+	// no JWT ==> {unknown}.{unknown}
 	jwt := parseJwtBody(r)
 	if jwt == nil {
-		return unknownRealmClient
+		return unknownUnknown
 	}
 
-	// no realm in JWT ==>  <unknown>.<unknown>
+	// no realm in JWT ==> {unknown}.{unknown}
 	realm, ok := jwt.getOneOfString(f.Spec.realmKeys)
 	if !ok {
-		return unknownRealmClient
+		return unknownUnknown
 	}
 
-	// no matcher ==>  realm.<unknown>
-	if path.ClientTracking.ClientTrackingMatcher == nil {
-		return realm + unknownClientAfterKnownRealm
+	// realm is not one of the realmsTrackingPattern to be tracked ==> realm.{all}
+	if !path.ClientTracking.RealmsTrackingMatcher.MatchString(realm) {
+		return realm + ".{all}"
 	}
 
-	// no client in JWT ==>  realm.<unknown>
+	// no client in JWT ==> realm.{unknown}
 	client, ok := jwt.getOneOfString(f.Spec.clientKeys)
 	if !ok {
-		return realm + unknownClientAfterKnownRealm
+		return realm + "." + unknownPlaceholder
 	}
 
-	// if `realm.client` does not match ==>  realm.<unknown>
-	realmAndClient := realm + "." + client
-	if !path.ClientTracking.ClientTrackingMatcher.MatchString(realmAndClient) {
-		return realm + unknownClientAfterKnownRealm
+	// if client does not match ==> realm.{no-match}
+	if !path.ClientTracking.ClientTrackingMatcher.MatchString(client) {
+		return realm + "." + noMatchPlaceholder
 	}
 
-	// all matched ==>  realm.client
-	return realmAndClient
+	// all matched ==> realm.client
+	return realm + "." + client
 }
 
 // resolvePath tries to match the request's path with one of the configured path template.
@@ -163,7 +158,7 @@ func getEndpointMetricsNames(req *http.Request, path *pathInfo) *endpointMetricN
 	methodIndex, ok := methodToIndex[method]
 	if !ok {
 		methodIndex = methodIndexUnknown
-		method = unknownElementPlaceholder
+		method = unknownPlaceholder
 	}
 
 	if p := path.metricPrefixesPerMethod[methodIndex]; p != nil {
@@ -174,7 +169,7 @@ func getEndpointMetricsNames(req *http.Request, path *pathInfo) *endpointMetricN
 
 // createAndCacheMetricsNames generates metrics names and cache them.
 func createAndCacheMetricsNames(path *pathInfo, method string, methodIndex int) *endpointMetricNames {
-	endpointPrefix := path.CommonPrefix + method + "." + path.PathTemplate + ".*.*."
+	endpointPrefix := path.CommonPrefix + method + "." + path.PathLabel + ".*.*."
 	prefixes := &endpointMetricNames{
 		endpointPrefix: endpointPrefix,
 		countAll:       endpointPrefix + metricCountAll,