Skip to content

Commit

Permalink
feat(api-usage-monitoring-filter): improvements proposed by @tkrop (#949
Browse files Browse the repository at this point in the history 
)

Signed-off-by: Maxim Tschumak <maxim.tschumak@gmail.com>
  • Loading branch information
@maxim-tschumak
maxim-tschumak committed Feb 12, 2019
1 parent eb5ea86 commit cc7cc67
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 36 deletions.
14 changes: 12 additions & 2 deletions docs/reference/filters.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1191,7 +1191,7 @@ apiUsageMonitoring(`
"foo/orders/:order-id",
"foo/orders/:order-id/order_item/{order-item-id}"
],
"client_tracking_pattern": "(joe|sabine)"
"client_tracking_pattern": "(shipping\-service|payment\-service)"
}`,`{
"application_id": "my-app",
"api_id": "customers-api",
Expand Down Expand Up @@ -1225,4 +1225,14 @@ 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
and path template.

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
18 changes: 9 additions & 9 deletions filters/apiusagemonitoring/filter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,40 +104,40 @@ func (f *apiUsageMonitoringFilter) getClientMetricsNames(realmClientKey string,
return prefixes
}

const format = "%s.%s"
const unknownUnknown = unknownPlaceholder + "." + unknownPlaceholder

// 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 fmt.Sprintf(format, unknown, unknown)
return unknownUnknown
}

// no realm in JWT ==> {unknown}.{unknown}
realm, ok := jwt.getOneOfString(f.Spec.realmKeys)
if !ok {
return fmt.Sprintf(format, unknown, unknown)
return unknownUnknown
}

// realm is not one of the realms to be tracked ==> realm.{all}
if !contains(path.ClientTracking.RealmsToTrack, realm) {
return fmt.Sprintf(format, realm, "{all}")
return realm + ".{all}"
}

// no client in JWT ==> realm.{unknown}
client, ok := jwt.getOneOfString(f.Spec.clientKeys)
if !ok {
return fmt.Sprintf(format, realm, unknown)
return realm + "." + unknownPlaceholder
}

// if client does not match ==> realm.{no-match}
if !path.ClientTracking.ClientTrackingMatcher.MatchString(client) {
return fmt.Sprintf(format, realm, noMatch)
return realm + "." + noMatchPlaceholder
}

// all matched ==> realm.client
return fmt.Sprintf(format, realm, client)
return realm + "." + client
}

func contains(strings []string, s string) bool {
Expand Down Expand Up @@ -167,7 +167,7 @@ func getEndpointMetricsNames(req *http.Request, path *pathInfo) *endpointMetricN
methodIndex, ok := methodToIndex[method]
if !ok {
methodIndex = methodIndexUnknown
method = unknown
method = unknownPlaceholder
}

if p := path.metricPrefixesPerMethod[methodIndex]; p != nil {
Expand All @@ -178,7 +178,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,
Expand Down
6 changes: 3 additions & 3 deletions filters/apiusagemonitoring/filter_endpointmetrics_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -318,8 +318,8 @@ func Test_Filter_PathTemplateMatchesInternalSlashesTooFollowingVarPart(t *testin
return spec.CreateFilter(args)
}
for _, c := range []struct {
requestPath string
expectedMatchedPathTemplate string
requestPath string
expectedMatchedPathLabel string
}{
{"foo/1", "foo/{a}"},
{"foo/1/2", "foo/{a}/{b}"},
Expand All @@ -336,7 +336,7 @@ func Test_Filter_PathTemplateMatchesInternalSlashesTooFollowingVarPart(t *testin
fmt.Sprintf("https://www.example.org/%s", c.requestPath),
204,
func(t *testing.T, pass int, m *metricstest.MockMetrics) {
pre := "apiUsageMonitoring.custom.my_app.my_api.GET." + c.expectedMatchedPathTemplate + ".*.*."
pre := "apiUsageMonitoring.custom.my_app.my_api.GET." + c.expectedMatchedPathLabel + ".*.*."
m.WithCounters(func(counters map[string]int64) {
assert.Equal(t,
map[string]int64{
Expand Down
8 changes: 5 additions & 3 deletions filters/apiusagemonitoring/pathinfo.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ type pathInfo struct {
ApplicationId string
ApiId string
PathTemplate string
PathLabel string
Matcher *regexp.Regexp
ClientTracking *clientTrackingInfo
CommonPrefix string
Expand All @@ -19,7 +20,7 @@ type pathInfo struct {
metricPrefixedPerClient map[string]*clientMetricNames
}

func newPathInfo(applicationId, apiId, pathTemplate string, clientTracking *clientTrackingInfo) *pathInfo {
func newPathInfo(applicationId, apiId, pathTemplate string, pathLabel string, clientTracking *clientTrackingInfo) *pathInfo {
commonPrefix := applicationId + "." + apiId + "."
var metricPrefixedPerClient map[string]*clientMetricNames
if clientTracking != nil {
Expand All @@ -29,6 +30,7 @@ func newPathInfo(applicationId, apiId, pathTemplate string, clientTracking *clie
ApplicationId: applicationId,
ApiId: apiId,
PathTemplate: pathTemplate,
PathLabel: pathLabel,
metricPrefixedPerClient: metricPrefixedPerClient,
ClientTracking: clientTracking,
CommonPrefix: commonPrefix,
Expand Down Expand Up @@ -69,8 +71,8 @@ const (
methodIndexOptions // OPTIONS
methodIndexTrace // TRACE

methodIndexUnknown // Value when the HTTP Method is not in the known list
methodIndexLength // Gives the constant size of the `metricPrefixesPerMethod` array.
methodIndexUnknown // Value when the HTTP Method is not in the known list
methodIndexLength // Gives the constant size of the `metricPrefixesPerMethod` array.
)

var (
Expand Down
28 changes: 18 additions & 10 deletions filters/apiusagemonitoring/spec.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ import (
const (
Name = "apiUsageMonitoring"

unknown = "{unknown}"
noMatch = "{no-match}"
unknownPlaceholder = "{unknown}"
noMatchPlaceholder = "{no-match}"

regexUrlPathPart = `.+`
regexOptionalSlashes = `\/*`
Expand Down Expand Up @@ -72,9 +72,10 @@ func NewApiUsageMonitoring(
}
}
unknownPath := newPathInfo(
unknown,
unknown,
noMatch,
unknownPlaceholder,
unknownPlaceholder,
noMatchPlaceholder,
noMatchPlaceholder,
unknownPathClientTracking,
)
spec := &apiUsageMonitoringSpec{
Expand Down Expand Up @@ -158,6 +159,7 @@ func (s *apiUsageMonitoringSpec) buildUnknownPathInfo(paths []*pathInfo) *pathIn
return newPathInfo(
*applicationId,
s.unknownPath.ApiId,
s.unknownPath.PathLabel,
s.unknownPath.PathTemplate,
s.unknownPath.ClientTracking)
}
Expand All @@ -171,7 +173,7 @@ func (s *apiUsageMonitoringSpec) buildPathInfoListFromConfiguration(apis []*apiC
for apiIndex, api := range apis {

applicationId := api.ApplicationId
if api.ApplicationId == "" {
if applicationId == "" {
log.Errorf("args[%d] ignored: does not specify an application_id", apiIndex)
continue
}
Expand Down Expand Up @@ -200,10 +202,10 @@ func (s *apiUsageMonitoringSpec) buildPathInfoListFromConfiguration(apis []*apiC
}

// Normalize path template and get regular expression from it
normalisedPathTemplate, regExStr := generateRegExpStringForPathTemplate(template)
normalisedPathTemplate, regExStr, pathLabel := generateRegExpStringForPathTemplate(template)

// Create new `pathInfo` with normalized PathTemplate
info := newPathInfo(applicationId, apiId, normalisedPathTemplate, clientTrackingInfo)
info := newPathInfo(applicationId, apiId, normalisedPathTemplate, pathLabel, clientTrackingInfo)

// Detect path template duplicates
if _, ok := existingPathTemplates[info.PathTemplate]; ok {
Expand Down Expand Up @@ -280,10 +282,11 @@ func (s *apiUsageMonitoringSpec) buildClientTrackingInfo(apiIndex int, api *apiC

// generateRegExpStringForPathTemplate normalizes the given path template and
// creates a regular expression from it.
func generateRegExpStringForPathTemplate(pathTemplate string) (normalizedPathTemplate, matcher string) {
func generateRegExpStringForPathTemplate(pathTemplate string) (normalizedPathTemplate, matcher, pathLabel string) {
pathParts := strings.Split(pathTemplate, "/")
matcherPathParts := make([]string, 0, len(pathParts))
normalizedPathTemplateParts := make([]string, 0, len(pathParts))
pathLabelParts := make([]string, 0, len(pathParts))
for _, p := range pathParts {
if p == "" {
continue
Expand All @@ -293,10 +296,13 @@ func generateRegExpStringForPathTemplate(pathTemplate string) (normalizedPathTem
// this part is not a placeholder: match it exactly
matcherPathParts = append(matcherPathParts, p)
normalizedPathTemplateParts = append(normalizedPathTemplateParts, p)
pathLabelParts = append(pathLabelParts, p)

} else {
// this part is a placeholder: match a wildcard for it
matcherPathParts = append(matcherPathParts, regexUrlPathPart)
normalizedPathTemplateParts = append(normalizedPathTemplateParts, fmt.Sprintf("{%s}", placeholderName))
normalizedPathTemplateParts = append(normalizedPathTemplateParts, ":"+placeholderName)
pathLabelParts = append(pathLabelParts, "{"+placeholderName+"}")
}
}
rawRegEx := &strings.Builder{}
Expand All @@ -308,6 +314,8 @@ func generateRegExpStringForPathTemplate(pathTemplate string) (normalizedPathTem

matcher = rawRegEx.String()
normalizedPathTemplate = strings.Join(normalizedPathTemplateParts, "/")
pathLabel = strings.Join(pathLabelParts, "/")

return
}

Expand Down
18 changes: 9 additions & 9 deletions filters/apiusagemonitoring/spec_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,13 +233,13 @@ func Test_CreateFilter_FullConfigSingleApi(t *testing.T) {
{
ApplicationId: "my_app",
ApiId: "my_api",
PathTemplate: "foo/orders/{order-id}/order_item/{order-item-id}",
PathTemplate: "foo/orders/:order-id/order_item/:order-item-id",
Matcher: matcher("^\\/*foo\\/orders\\/.+\\/order_item\\/.+\\/*$"),
},
{
ApplicationId: "my_app",
ApiId: "my_api",
PathTemplate: "foo/orders/{order-id}",
PathTemplate: "foo/orders/:order-id",
Matcher: matcher("^\\/*foo\\/orders\\/.+\\/*$"),
},
{
Expand All @@ -251,7 +251,7 @@ func Test_CreateFilter_FullConfigSingleApi(t *testing.T) {
{
ApplicationId: "my_app",
ApiId: "my_api",
PathTemplate: "foo/customers/{customer-id}",
PathTemplate: "foo/customers/:customer-id",
Matcher: matcher("^\\/*foo\\/customers\\/.+\\/*$"),
},
{
Expand Down Expand Up @@ -320,13 +320,13 @@ func Test_CreateFilter_FullConfigMultipleApis(t *testing.T) {
{
ApplicationId: "my_app",
ApiId: "orders_api",
PathTemplate: "foo/orders/{order-id}/order_item/{order-item-id}",
PathTemplate: "foo/orders/:order-id/order_item/:order-item-id",
Matcher: matcher("^\\/*foo\\/orders\\/.+\\/order_item\\/.+\\/*$"),
},
{
ApplicationId: "my_app",
ApiId: "orders_api",
PathTemplate: "foo/orders/{order-id}",
PathTemplate: "foo/orders/:order-id",
Matcher: matcher("^\\/*foo\\/orders\\/.+\\/*$"),
},
{
Expand All @@ -338,7 +338,7 @@ func Test_CreateFilter_FullConfigMultipleApis(t *testing.T) {
{
ApplicationId: "my_app",
ApiId: "customers_api",
PathTemplate: "foo/customers/{customer-id}",
PathTemplate: "foo/customers/:customer-id",
Matcher: matcher("^\\/*foo\\/customers\\/.+\\/*$"),
},
{
Expand Down Expand Up @@ -375,13 +375,13 @@ func Test_CreateFilter_FullConfigWithApisWithoutPaths(t *testing.T) {
{
ApplicationId: "my_order_app",
ApiId: "orders_api",
PathTemplate: "foo/orders/{order-id}/order_item/{order-item-id}",
PathTemplate: "foo/orders/:order-id/order_item/:order-item-id",
Matcher: matcher("^\\/*foo\\/orders\\/.+\\/order_item\\/.+\\/*$"),
},
{
ApplicationId: "my_order_app",
ApiId: "orders_api",
PathTemplate: "foo/orders/{order-id}",
PathTemplate: "foo/orders/:order-id",
Matcher: matcher("^\\/*foo\\/orders\\/.+\\/*$"),
},
{
Expand Down Expand Up @@ -434,7 +434,7 @@ func Test_CreateFilter_DuplicateMatchersAreIgnored(t *testing.T) {
{
ApplicationId: "my_app",
ApiId: "orders_api",
PathTemplate: "foo/{a}",
PathTemplate: "foo/:a",
Matcher: matcher("^\\/*foo\\/.+\\/*$"),
},
})
Expand Down

0 comments on commit cc7cc67

Please sign in to comment.