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.

Sign up for GitHub

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

Jump to bottom

Add AC hit rate metrics with prometheus labels (alternative implementation with decorator) #358

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add AC hit rate metrics with prometheus labels (alternative implementation with decorator) #358

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b2b8b2
Add AC hit rate metrics with prometheus labels
ulrfa May 28, 2020
7f54961
refactor cache interfaces
ulrfa Sep 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions cache/cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,30 +68,35 @@ type RequestContext interface {
}

// TODO Document interface
type CasAcCache interface {
type BlobStore interface {
// TODO change to io.ReadCloser?
Put(kind EntryKind, hash string, size int64, rdr io.Reader, reqCtx RequestContext) error
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 we could use a map[string]string instead of a RequestContext interface in these functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that first, but ended up with cache.RequestContext because:

  • gRPC and HTTP/2 is using canonical lowercase header names, but the golang HTTP/1.1 library normalize header names to start with capital letter. I want to hide that difference, but avoid translating headers that are never accessed, and instead translate on demand in:

    bazel-remote/server/http.go

    Lines 395 to 398 in 4b2b8b2

    func (h *reqCtxHttp) GetHeader(headerName string) (headerValues []string) {
    headerName = strings.Title(headerName)
    if headerValues, ok := h.request.Header[headerName]; ok {
    return headerValues

  • Minimize overhead for creating cache.RequestContext instance in general (only allocate struct and store a pointer)

  • Allows extending cache.RequestContext with additional methods that returns non-header meta information. Such as Host, RemoteAddr or Cookie from HTTP requests. Or any other information that could be added by bazel-remote’s HTTP/GRPC servers, like enum indicating if original request were HTTP or GRPC.

  • Allows extending cache.RequestContext to allow proxies to cancel outgoing requests, if associated incomming request is canceled. I have not looked into details, but seems to be something related to that in underlying GRPC and HTTP contexts.

  • Allows extending cache.RequestContext in general, instead of having to update all Get/Put/Contains invocations with additional parameters, all over bazel-remote codebase.

  • Allows propagating custom information in general between a chain of different cache.BlobStore implementations (such as proxies, decorators, …)


Get(kind EntryKind, hash string, size int64, reqCtx RequestContext) (io.ReadCloser, int64, error)

Contains(kind EntryKind, hash string, size int64, reqCtx RequestContext) (bool, int64)
}

// TODO Document interface
type AcStore interface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this interface used anywhere outside BlobAcStore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I’m only thinking that having a separate AcStore interface might be more generic and flexible. Especially if GetValidatedActionResult method would be extracted from disk.go in the future. Or if there would be decorators wrapping only AcStore. But I don’t have much experience of embedding interfaces and structs in golang. It might work with only BlobStore and BlobAcStore interface, if you think it is preferable to avoid AcStore?

GetValidatedActionResult(hash string, reqCtx RequestContext) (*pb.ActionResult, []byte, error)
}

// TODO Document interface
type BlobAcStore interface {
BlobStore
AcStore
}

// TODO Document interface
type Stats interface {
Stats() (totalSize int64, reservedSize int64, numItems int)
MaxSize() int64
}

// TODO Should the proxy interface also be extended with RequestContext parameter? To allow
// for example forwarding of custom headers from client to proxy, or support for HTTP
// headers like Max-Forwards.

// TODO Could the disk and proxies implement same interface? But proxies are not supporting
// GetValidatedActionResult and that method is important to have in the interface
// for cache.metricdecorator.
// TODO Could the proxies implement the BlobStore interface instead? And remove Proxy interface?
// Having access to the original headers would allow new use cases such as forwarding of
// custom headers from client via proxy, or support for HTTP headers like Max-Forwards.

// Proxy is the interface that (optional) proxy backends must implement.
// Implementations are expected to be safe for concurrent use.
Expand Down
3 changes: 3 additions & 0 deletions cache/disk/disk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,6 +649,9 @@ func cacheFilePath(kind cache.EntryKind, cacheDir string, hash string) string {
// value from the CAS if it and all its dependencies are also available. If
// not, nil values are returned. If something unexpected went wrong, return
// an error.
// TODO Consider separating implementation of cache.AcStore interface, to open up
// possibilities combining that functionality with proxies, and also allow
// bazel-remote configurations with proxy but no local disk storage?
Copy link
Collaborator

Choose a reason for hiding this comment

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

A proxy-only mode has been on my todo-list for a while. I have considered doing this as a short-circuit inside disk.Cache (it might be time to rename or split the package at that point).

Copy link
Contributor

Choose a reason for hiding this comment

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

@mostynb Does it make sense to bring back the cache.Cache interface that the proxies and disk cache used to implement for something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mostynb Is it something more than implementation of GetValidatedActionResult, that you expect will be needed from disk.io, in a proxy-only-mode?

func (c *Cache) GetValidatedActionResult(hash string, reqCtx cache.RequestContext) (*pb.ActionResult, []byte, error) {

rc, sizeBytes, err := c.Get(cache.AC, hash, -1, reqCtx)
Expand Down
16 changes: 10 additions & 6 deletions cache/metricsdecorator/metricsdecorator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
package metricsdecorator

// This is a decorator for any implementation of the cache.CasAcCache interface.
// This is a decorator for any implementation of the cache.BlobAcStore interface.
// It adds prometheus metrics for the cache requests.
//
// The decorator can report cache miss if AC is found but referenced CAS entries are missing.
// That is possible since GetValidatedActionResult is part of the cache.CasAcCache
// interface, not only the low level Get method.

// That is possible since metricsdecorator supports GetValidatedActionResult in the
// cache.BlobAcStore interface.
//
// TODO Consider allow using a metricsdecorator also for pure cache.BlobStore interfaces,
// in order to replace the current prometheus counters in the proxies? That would
// probably require better support for non AC requests in metricsdecorator and configurable
// counter name.
import (
"github.com/buchgr/bazel-remote/cache"
"github.com/buchgr/bazel-remote/config"
Expand All @@ -21,7 +25,7 @@ import (
type metrics struct {
categoryValues map[string]map[string]struct{}
counterIncomingReqs *prometheus.CounterVec
parent cache.CasAcCache
parent cache.BlobAcStore
}

const statusOK = "ok"
Expand All @@ -34,7 +38,7 @@ const methodContains = "contains"

// TODO add test cases for this file

func NewMetricsDecorator(config *config.Metrics, parent cache.CasAcCache) cache.CasAcCache {
func NewMetricsDecorator(config *config.Metrics, parent cache.BlobAcStore) cache.BlobAcStore {

labels := []string{"method", "status", "kind"}
categoryValues := make(map[string]map[string]struct{})
Expand Down
6 changes: 3 additions & 3 deletions server/grpc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ var (
)

type grpcServer struct {
cache cache.CasAcCache
cache cache.BlobAcStore
accessLogger cache.Logger
errorLogger cache.Logger
depsCheck bool
Expand All @@ -48,7 +48,7 @@ func ListenAndServeGRPC(addr string, opts []grpc.ServerOption,
validateACDeps bool,
mangleACKeys bool,
enableRemoteAssetAPI bool,
c cache.CasAcCache, a cache.Logger, e cache.Logger) error {
c cache.BlobAcStore, a cache.Logger, e cache.Logger) error {

listener, err := net.Listen("tcp", addr)
if err != nil {
Expand All @@ -62,7 +62,7 @@ func serveGRPC(l net.Listener, opts []grpc.ServerOption,
validateACDepsCheck bool,
mangleACKeys bool,
enableRemoteAssetAPI bool,
c cache.CasAcCache, a cache.Logger, e cache.Logger) error {
c cache.BlobAcStore, a cache.Logger, e cache.Logger) error {

srv := grpc.NewServer(opts...)
s := &grpcServer{
Expand Down
4 changes: 2 additions & 2 deletions server/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ type HTTPCache interface {
}

type httpCache struct {
cache cache.CasAcCache
cache cache.BlobAcStore
stats cache.Stats
accessLogger cache.Logger
errorLogger cache.Logger
Expand All @@ -51,7 +51,7 @@ type statusPageData struct {
// accessLogger will print one line for each HTTP request to stdout.
// errorLogger will print unexpected server errors. Inexistent files and malformed URLs will not
// be reported.
func NewHTTPCache(cache cache.CasAcCache, stats cache.Stats, accessLogger cache.Logger, errorLogger cache.Logger, validateAC bool, mangleACKeys bool, commit string) HTTPCache {
func NewHTTPCache(cache cache.BlobAcStore, stats cache.Stats, accessLogger cache.Logger, errorLogger cache.Logger, validateAC bool, mangleACKeys bool, commit string) HTTPCache {
_, _, numItems := stats.Stats()

errorLogger.Printf("Loaded %d existing disk cache items.", numItems)
Expand Down