Skip to content

Commit

Permalink
Add metrics for outbound API calls (#1360)
Browse files Browse the repository at this point in the history
* Add metrics for outbound API calls

Initially I tried to use
https://pkg.go.dev/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp#NewTransport
but that seems to just create a new span. I was never able to get any
useful metrics out of the endpoint even though I verified I was actually
using the instrumented transport. We're using `otelhttp` already for
instrumenting handlers where it works fine, but I wasn't able to get the
client side working.

So I added a custom transport that records the duration of the request
along with the method and the status code.

The resulting historam looks like this:
```
mediator_github_http_roundtrip_duration_milliseconds_bucket{http_method="GET",http_status_code="200",otel_scope_name="providers",otel_scope_version="",le="0"}
0
mediator_github_http_roundtrip_duration_milliseconds_bucket{http_method="GET",http_status_code="200",otel_scope_name="providers",otel_scope_version="",le="5"}
0
mediator_github_http_roundtrip_duration_milliseconds_bucket{http_method="GET",http_status_code="200",otel_scope_name="providers",otel_scope_version="",le="10"}
0
mediator_github_http_roundtrip_duration_milliseconds_bucket{http_method="GET",http_status_code="200",otel_scope_name="providers",otel_scope_version="",le="25"}
0
mediator_github_http_roundtrip_duration_milliseconds_bucket{http_method="GET",http_status_code="200",otel_scope_name="providers",otel_scope_version="",le="50"}
0
mediator_github_http_roundtrip_duration_milliseconds_bucket{http_method="GET",http_status_code="200",otel_scope_name="providers",otel_scope_version="",le="75"}
0
mediator_github_http_roundtrip_duration_milliseconds_bucket{http_method="GET",http_status_code="200",otel_scope_name="providers",otel_scope_version="",le="100"}
0
mediator_github_http_roundtrip_duration_milliseconds_bucket{http_method="GET",http_status_code="200",otel_scope_name="providers",otel_scope_version="",le="250"}
4
mediator_github_http_roundtrip_duration_milliseconds_bucket{http_method="GET",http_status_code="200",otel_scope_name="providers",otel_scope_version="",le="500"}
7
mediator_github_http_roundtrip_duration_milliseconds_bucket{http_method="GET",http_status_code="200",otel_scope_name="providers",otel_scope_version="",le="750"}
7
mediator_github_http_roundtrip_duration_milliseconds_bucket{http_method="GET",http_status_code="200",otel_scope_name="providers",otel_scope_version="",le="1000"}
7
mediator_github_http_roundtrip_duration_milliseconds_bucket{http_method="GET",http_status_code="200",otel_scope_name="providers",otel_scope_version="",le="2500"}
7
mediator_github_http_roundtrip_duration_milliseconds_bucket{http_method="GET",http_status_code="200",otel_scope_name="providers",otel_scope_version="",le="5000"}
7
mediator_github_http_roundtrip_duration_milliseconds_bucket{http_method="GET",http_status_code="200",otel_scope_name="providers",otel_scope_version="",le="7500"}
7
mediator_github_http_roundtrip_duration_milliseconds_bucket{http_method="GET",http_status_code="200",otel_scope_name="providers",otel_scope_version="",le="10000"}
7
mediator_github_http_roundtrip_duration_milliseconds_bucket{http_method="GET",http_status_code="200",otel_scope_name="providers",otel_scope_version="",le="+Inf"}
7
mediator_github_http_roundtrip_duration_milliseconds_sum{http_method="GET",http_status_code="200",otel_scope_name="providers",otel_scope_version=""}
1709
mediator_github_http_roundtrip_duration_milliseconds_count{http_method="GET",http_status_code="200",otel_scope_name="providers",otel_scope_version=""}
7
```

* Add r.URL.Host as another label

* Use xsync.MapOf

* functional option pattern for NewProviderBuilder

* Use the functional pattern for server,reconciler and executor, too
  • Loading branch information
jhrozek authored Nov 2, 2023
1 parent 534e861 commit 81ba458
Show file tree
Hide file tree
Showing 14 changed files with 351 additions and 24 deletions.
8 changes: 5 additions & 3 deletions cmd/server/app/serve.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"github.com/stacklok/mediator/internal/engine"
"github.com/stacklok/mediator/internal/events"
"github.com/stacklok/mediator/internal/logger"
provtelemetry "github.com/stacklok/mediator/internal/providers/telemetry"
"github.com/stacklok/mediator/internal/reconcilers"
)

Expand Down Expand Up @@ -101,20 +102,21 @@ var serveCmd = &cobra.Command{
}

serverMetrics := controlplane.NewMetrics()
providerMetrics := provtelemetry.NewProviderMetrics()

s, err := controlplane.NewServer(store, evt, serverMetrics, cfg, vldtr)
s, err := controlplane.NewServer(store, evt, serverMetrics, cfg, vldtr, controlplane.WithProviderMetrics(providerMetrics))
if err != nil {
return fmt.Errorf("unable to create server: %w", err)
}

exec, err := engine.NewExecutor(store, &cfg.Auth)
exec, err := engine.NewExecutor(store, &cfg.Auth, engine.WithProviderMetrics(providerMetrics))
if err != nil {
return fmt.Errorf("unable to create executor: %w", err)
}

s.ConsumeEvents(exec)

rec, err := reconcilers.NewReconciler(store, evt, &cfg.Auth)
rec, err := reconcilers.NewReconciler(store, evt, &cfg.Auth, reconcilers.WithProviderMetrics(providerMetrics))
if err != nil {
return fmt.Errorf("unable to create reconciler: %w", err)
}
Expand Down
5 changes: 4 additions & 1 deletion internal/controlplane/handlers_githubwebhooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,10 @@ func (s *Server) parseGithubEventForProcessing(
return fmt.Errorf("error getting provider: %w", err)
}

provBuilder, err := providers.GetProviderBuilder(ctx, prov, dbRepo.ProjectID, s.store, s.cryptoEngine)
pbOpts := []providers.ProviderBuilderOption{
providers.WithProviderMetrics(s.provMt),
}
provBuilder, err := providers.GetProviderBuilder(ctx, prov, dbRepo.ProjectID, s.store, s.cryptoEngine, pbOpts...)
if err != nil {
return fmt.Errorf("error building client: %w", err)
}
Expand Down
10 changes: 8 additions & 2 deletions internal/controlplane/handlers_repositories.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,10 @@ func (s *Server) RegisterRepository(ctx context.Context,
return nil, providerError(fmt.Errorf("provider error: %w", err))
}

p, err := providers.GetProviderBuilder(ctx, provider, projectID, s.store, s.cryptoEngine)
pbOpts := []providers.ProviderBuilderOption{
providers.WithProviderMetrics(s.provMt),
}
p, err := providers.GetProviderBuilder(ctx, provider, projectID, s.store, s.cryptoEngine, pbOpts...)
if err != nil {
return nil, status.Errorf(codes.Internal, "cannot get provider builder: %v", err)
}
Expand Down Expand Up @@ -452,7 +455,10 @@ func (s *Server) ListRemoteRepositoriesFromProvider(
return nil, status.Errorf(codes.PermissionDenied, "cannot get access token for provider")
}

p, err := providers.GetProviderBuilder(ctx, provider, projectID, s.store, s.cryptoEngine)
pbOpts := []providers.ProviderBuilderOption{
providers.WithProviderMetrics(s.provMt),
}
p, err := providers.GetProviderBuilder(ctx, provider, projectID, s.store, s.cryptoEngine, pbOpts...)
if err != nil {
return nil, status.Errorf(codes.Internal, "cannot get provider builder: %v", err)
}
Expand Down
32 changes: 29 additions & 3 deletions internal/controlplane/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import (
"github.com/stacklok/mediator/internal/db"
"github.com/stacklok/mediator/internal/events"
"github.com/stacklok/mediator/internal/logger"
provtelemetry "github.com/stacklok/mediator/internal/providers/telemetry"
"github.com/stacklok/mediator/internal/util"
legacy "github.com/stacklok/mediator/pkg/api/protobuf/go/mediator/v1"
pb "github.com/stacklok/mediator/pkg/api/protobuf/go/minder/v1"
Expand All @@ -67,6 +68,7 @@ type Server struct {
cfg *config.Config
evt *events.Eventer
mt *metrics
provMt provtelemetry.ProviderMetrics
grpcServer *grpc.Server
vldtr auth.JwtValidator
pb.UnimplementedHealthServiceServer
Expand All @@ -89,20 +91,44 @@ type Server struct {
cryptoEngine *crypto.Engine
}

// ServerOption is a function that modifies a server
type ServerOption func(*Server)

// WithProviderMetrics sets the provider metrics for the server
func WithProviderMetrics(mt provtelemetry.ProviderMetrics) ServerOption {
return func(s *Server) {
s.provMt = mt
}
}

// NewServer creates a new server instance
func NewServer(store db.Store, evt *events.Eventer, cpm *metrics, cfg *config.Config, vldtr auth.JwtValidator) (*Server, error) {
func NewServer(
store db.Store,
evt *events.Eventer,
cpm *metrics,
cfg *config.Config,
vldtr auth.JwtValidator,
opts ...ServerOption,
) (*Server, error) {
eng, err := crypto.EngineFromAuthConfig(&cfg.Auth)
if err != nil {
return nil, fmt.Errorf("failed to create crypto engine: %w", err)
}
return &Server{
s := &Server{
store: store,
cfg: cfg,
evt: evt,
cryptoEngine: eng,
vldtr: vldtr,
mt: cpm,
}, nil
provMt: provtelemetry.NewNoopMetrics(),
}

for _, opt := range opts {
opt(s)
}

return s, nil
}

var _ (events.Registrar) = (*Server)(nil)
Expand Down
34 changes: 30 additions & 4 deletions internal/engine/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
engif "github.com/stacklok/mediator/internal/engine/interfaces"
"github.com/stacklok/mediator/internal/events"
"github.com/stacklok/mediator/internal/providers"
providertelemetry "github.com/stacklok/mediator/internal/providers/telemetry"
pb "github.com/stacklok/mediator/pkg/api/protobuf/go/minder/v1"
)

Expand All @@ -42,19 +43,41 @@ const (
type Executor struct {
querier db.Store
crypteng *crypto.Engine
provMt providertelemetry.ProviderMetrics
}

// ExecutorOption is a function that modifies an executor
type ExecutorOption func(*Executor)

// WithProviderMetrics sets the provider metrics for the executor
func WithProviderMetrics(mt providertelemetry.ProviderMetrics) ExecutorOption {
return func(e *Executor) {
e.provMt = mt
}
}

// NewExecutor creates a new executor
func NewExecutor(querier db.Store, authCfg *config.AuthConfig) (*Executor, error) {
func NewExecutor(
querier db.Store,
authCfg *config.AuthConfig,
opts ...ExecutorOption,
) (*Executor, error) {
crypteng, err := crypto.EngineFromAuthConfig(authCfg)
if err != nil {
return nil, err
}

return &Executor{
e := &Executor{
querier: querier,
crypteng: crypteng,
}, nil
provMt: providertelemetry.NewNoopMetrics(),
}

for _, opt := range opts {
opt(e)
}

return e, nil
}

// Register implements the Consumer interface.
Expand Down Expand Up @@ -89,7 +112,10 @@ func (e *Executor) HandleEntityEvent(msg *message.Message) error {
return fmt.Errorf("error getting provider: %w", err)
}

cli, err := providers.GetProviderBuilder(ctx, provider, *projectID, e.querier, e.crypteng)
pbOpts := []providers.ProviderBuilderOption{
providers.WithProviderMetrics(e.provMt),
}
cli, err := providers.GetProviderBuilder(ctx, provider, *projectID, e.querier, e.crypteng, pbOpts...)
if err != nil {
return fmt.Errorf("error building client: %w", err)
}
Expand Down
9 changes: 9 additions & 0 deletions internal/providers/github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"golang.org/x/oauth2"

"github.com/stacklok/mediator/internal/db"
"github.com/stacklok/mediator/internal/providers/telemetry"
minderv1 "github.com/stacklok/mediator/pkg/api/protobuf/go/minder/v1"
provifv1 "github.com/stacklok/mediator/pkg/providers/v1"
)
Expand Down Expand Up @@ -63,14 +64,22 @@ var _ provifv1.GitHub = (*RestClient)(nil)
func NewRestClient(
ctx context.Context,
config *minderv1.GitHubProviderConfig,
metrics telemetry.HttpClientMetrics,
token string,
owner string,
) (*RestClient, error) {
var err error

ts := oauth2.StaticTokenSource(
&oauth2.Token{AccessToken: token},
)
tc := oauth2.NewClient(ctx, ts)

tc.Transport, err = metrics.NewDurationRoundTripper(tc.Transport, db.ProviderTypeGithub)
if err != nil {
return nil, fmt.Errorf("error creating duration round tripper: %w", err)
}

ghClient := github.NewClient(tc)

if config.Endpoint != "" {
Expand Down
6 changes: 5 additions & 1 deletion internal/providers/github/github_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (

"github.com/stretchr/testify/assert"

provtelemetry "github.com/stacklok/mediator/internal/providers/telemetry"
minderv1 "github.com/stacklok/mediator/pkg/api/protobuf/go/minder/v1"
)

Expand All @@ -28,7 +29,10 @@ func TestNewRestClient(t *testing.T) {

client, err := NewRestClient(context.Background(), &minderv1.GitHubProviderConfig{
Endpoint: "https://api.github.com",
}, "token", "")
},
provtelemetry.NewNoopMetrics(),
"token", "")

assert.NoError(t, err)
assert.NotNil(t, client)
}
16 changes: 14 additions & 2 deletions internal/providers/http/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import (

"golang.org/x/oauth2"

"github.com/stacklok/mediator/internal/db"
"github.com/stacklok/mediator/internal/providers/telemetry"
minderv1 "github.com/stacklok/mediator/pkg/api/protobuf/go/minder/v1"
provifv1 "github.com/stacklok/mediator/pkg/providers/v1"
)
Expand All @@ -40,8 +42,13 @@ type REST struct {
var _ provifv1.REST = (*REST)(nil)

// NewREST creates a new RESTful client.
func NewREST(config *minderv1.RESTProviderConfig, tok string) (*REST, error) {
func NewREST(
config *minderv1.RESTProviderConfig,
metrics telemetry.HttpClientMetrics,
tok string,
) (*REST, error) {
var cli *http.Client
var err error

if tok != "" {
ts := oauth2.StaticTokenSource(
Expand All @@ -52,8 +59,13 @@ func NewREST(config *minderv1.RESTProviderConfig, tok string) (*REST, error) {
cli = &http.Client{}
}

cli.Transport, err = metrics.NewDurationRoundTripper(cli.Transport, db.ProviderTypeRest)
if err != nil {
return nil, fmt.Errorf("error creating duration round tripper: %w", err)
}

var baseURL *url.URL
baseURL, err := baseURL.Parse(config.GetBaseUrl())
baseURL, err = baseURL.Parse(config.GetBaseUrl())
if err != nil {
return nil, err
}
Expand Down
29 changes: 25 additions & 4 deletions internal/providers/providers.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
gitclient "github.com/stacklok/mediator/internal/providers/git"
ghclient "github.com/stacklok/mediator/internal/providers/github"
httpclient "github.com/stacklok/mediator/internal/providers/http"
"github.com/stacklok/mediator/internal/providers/telemetry"
provinfv1 "github.com/stacklok/mediator/pkg/providers/v1"
)

Expand All @@ -39,6 +40,7 @@ func GetProviderBuilder(
projectID uuid.UUID,
store db.Store,
crypteng *crypto.Engine,
opts ...ProviderBuilderOption,
) (*ProviderBuilder, error) {
encToken, err := store.GetAccessTokenByProjectID(ctx,
db.GetAccessTokenByProjectIDParams{Provider: prov.Name, ProjectID: projectID})
Expand All @@ -51,7 +53,7 @@ func GetProviderBuilder(
return nil, fmt.Errorf("error decrypting access token: %w", err)
}

return NewProviderBuilder(&prov, encToken, decryptedToken.AccessToken), nil
return NewProviderBuilder(&prov, encToken, decryptedToken.AccessToken, opts...), nil
}

// ProviderBuilder is a utility struct which allows for the creation of
Expand All @@ -60,19 +62,38 @@ type ProviderBuilder struct {
p *db.Provider
tokenInf db.ProviderAccessToken
tok string
metrics telemetry.ProviderMetrics
}

// ProviderBuilderOption is a function which can be used to set options on the ProviderBuilder.
type ProviderBuilderOption func(*ProviderBuilder)

// WithProviderMetrics sets the metrics for the ProviderBuilder
func WithProviderMetrics(metrics telemetry.ProviderMetrics) ProviderBuilderOption {
return func(pb *ProviderBuilder) {
pb.metrics = metrics
}
}

// NewProviderBuilder creates a new provider builder.
func NewProviderBuilder(
p *db.Provider,
tokenInf db.ProviderAccessToken,
tok string,
opts ...ProviderBuilderOption,
) *ProviderBuilder {
return &ProviderBuilder{
pb := &ProviderBuilder{
p: p,
tokenInf: tokenInf,
tok: tok,
metrics: telemetry.NewNoopMetrics(),
}

for _, opt := range opts {
opt(pb)
}

return pb
}

// Implements returns true if the provider implements the given type.
Expand Down Expand Up @@ -123,7 +144,7 @@ func (pb *ProviderBuilder) GetHTTP(ctx context.Context) (provinfv1.REST, error)
return nil, fmt.Errorf("error parsing http config: %w", err)
}

return httpclient.NewREST(cfg, pb.tok)
return httpclient.NewREST(cfg, pb.metrics, pb.tok)
}

// GetGitHub returns a github client for the provider.
Expand All @@ -142,7 +163,7 @@ func (pb *ProviderBuilder) GetGitHub(ctx context.Context) (*ghclient.RestClient,
return nil, fmt.Errorf("error parsing github config: %w", err)
}

cli, err := ghclient.NewRestClient(ctx, cfg, pb.GetToken(), pb.tokenInf.OwnerFilter.String)
cli, err := ghclient.NewRestClient(ctx, cfg, pb.metrics, pb.GetToken(), pb.tokenInf.OwnerFilter.String)
if err != nil {
return nil, fmt.Errorf("error creating github client: %w", err)
}
Expand Down
Loading

0 comments on commit 81ba458

Please sign in to comment.