Skip to content
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 Prometheus metric categories #539

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ulrfa
Copy link
Contributor

@ulrfa ulrfa commented Apr 3, 2022

This pull request replaces my previous #350 and #358 PRs (@mostynb: feel free to close them if you want).

What do you think @mostynb? If you would like a subset of this PR, then I can restructure it. If you decide to not merge this PR that is also fine by me, since the context parameters and metrics decorator interface from the previous commits, allows me to maintain this as separate patch without too much effort.

cache/disk/BUILD.bazel Outdated Show resolved Hide resolved
@mostynb
Copy link
Collaborator

mostynb commented Apr 4, 2022

I'll try to take a look at this in the next day or two...

@ulrfa
Copy link
Contributor Author

ulrfa commented Apr 5, 2022

I'll try to take a look at this in the next day or two...

Thanks @mostynb! I'll let you have look before I address the current comments, since I assume reviewing is more convenient with fewer commits and without force-pushed commits.

@mostynb
Copy link
Collaborator

mostynb commented Apr 10, 2022

I have cherry-picked the FindMissingCasBlobs metrics fix to the master branch- good catch.

Re disabling Contains metrics, let's think this through a bit first, if it's not a big change I think it would be better to fix them than to disable them and then fix them. I have an idea for how to do this, that I need to run through in my head a bit. I think this should be dropped from this PR.

Re the metrics categories change, I'll try to take a look tomorrow.

See updated README.md file for description.
@ulrfa
Copy link
Contributor Author

ulrfa commented Apr 12, 2022

I have cherry-picked the FindMissingCasBlobs metrics fix to the master branch- good catch.

Thanks @mostynb!

Re disabling Contains metrics, let's think this through a bit first, if it's not a big change I think it would be better to fix them than to disable them and then fix them. I have an idea for how to do this, that I need to run through in my head a bit. I think this should be dropped from this PR.

OK, I removed that commit from this PR.

Re the metrics categories change, I'll try to take a look tomorrow.

Thanks @mostynb! I now force-pushed the branch to address the comments so far.

Copy link
Collaborator

@mostynb mostynb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry this took so long for me to get to. This change looks good, I only have one substantial suggestion (move http/grpc header processing into the http/grpc code in the server package).

Comment on lines +655 to +659
Received headers that match a declared category name, but with a value outside
the declared allowed value set, is reported with the value "other" to
Prometheus. This is convenient for categories such as "branch", where it from a
cache hit ratio perspective often make sense to distinguish between "main"
branch and "other" branches.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should mention here that only lowercase category names are allowed, otherwise it will likely be a common error that people will hit when first trying this feature. Also: are non lowercase values allowed?

if grpcHeaders != nil {
grpcHeaderValues := grpcHeaders[categoryNameLowerCase]
if len(grpcHeaderValues) > 0 {
// Pick the first header with matching name if multiple headers with same name
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be less surprising to pick the last matching header instead of the first?

// Pick the first header with matching name if multiple headers with same name
headerValue = grpcHeaderValues[0]
}
} else if httpHeaders != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking, when using grpc is it assumed that we should ignore http headers too?

}
} else if httpHeaders != nil {
headerValue = httpHeaders.Get(categoryNameLowerCase)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: add an empty line after this

@@ -351,6 +352,12 @@ func validateConfig(c *Config) error {
return errors.New("'access_log_level' must be set to either \"none\" or \"all\"")
}

for categoryName := range c.MetricCategories {
if categoryName != strings.ToLower(categoryName) {
return fmt.Errorf("Names in 'metric_categories' must be in lower case: %s", categoryName)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: reword to "metric_categories names must be lowercase: %q", categoryName

Comment on lines +210 to +222
// Retrieves HTTP headers from context. Minimizes type safety issues.
func getHttpHeaders(ctx context.Context) *http.Header {
headers, ok := ctx.Value(httpHeadersContextKey{}).(*http.Header)
if !ok {
return nil
}
return headers
}

func getGrpcHeaders(ctx context.Context) metadata.MD {
grpcHeaders, _ := metadata.FromIncomingContext(ctx)
return grpcHeaders
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be a little cleaner to find http/grpc headers in the server package (separately in http and grpc code), and then add them as a single custom value in the context? Then we could simplify addCategoryLabels a little.

@ulrfa
Copy link
Contributor Author

ulrfa commented Jun 22, 2022

Sorry, I have not had time to address these comments. I’m going on vacation now and I’m back again in middle of August. Wish you a good summer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants