Add metrics for outbound API calls #1360

jhrozek · 2023-10-31T19:55:36Z

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

jhrozek · 2023-10-31T19:57:31Z

cmd/dev/app/rule_type/rule_type.go

@@ -130,6 +131,7 @@ func testCmdRun(cmd *cobra.Command, _ []string) error {
 		},
 		db.ProviderAccessToken{},
 		token,
+		provtelemetry.NewNoopMetrics(),


I don't love the fact that I added a new parameter to a call that's used so widely. Maybe the provider builders could default to noop metrics and we could have an WithMetrics option that could be added where needed? The "typed" provider builders like NewRest or NewGithub would then require an explicit metric to be passed.

I actually really like this idea. Having a default NoopMetrics implementation and adding the implementation with a With function.

+1 to a functional options pattern.

Does this also mean that the rule type commands actually make calls to providers, or is that just an artifact of how we construct them?

the rule type command is a developer-facing command. It's instantiating the rule types in the same manner as the engine normally does and then the rule types are making the provider calls.

JAORMX · 2023-11-01T07:08:17Z

cmd/dev/app/rule_type/rule_type.go

@@ -130,6 +131,7 @@ func testCmdRun(cmd *cobra.Command, _ []string) error {
 		},
 		db.ProviderAccessToken{},
 		token,
+		provtelemetry.NewNoopMetrics(),


I actually really like this idea. Having a default NoopMetrics implementation and adding the implementation with a With function.

JAORMX · 2023-11-01T07:09:45Z

cmd/server/app/serve.go


-		s, err := controlplane.NewServer(store, evt, serverMetrics, cfg, vldtr)
+		s, err := controlplane.NewServer(store, evt, serverMetrics, providerMetrics, cfg, vldtr)


I think you could use the options pattern (WithMetrics or something of the sort) in the server, engine and reconcilers.

JAORMX

Where does the actual instrumentation happen? Is there some magic function that does that? cause I don't see that happening 😕

internal/providers/github/github.go

jhrozek · 2023-11-01T08:29:10Z

internal/providers/telemetry/telemetry.go

+}
+
+func (irt *instrumentedRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) {
+	startTime := time.Now()


every time a provider whose http client transport is wrapped using NewDurationRoundTripper makes an HTTP request, this method is called. All it does is to call the wrapped round trip and mark the time before and after and shove it into the histogram.

jhrozek · 2023-11-01T08:30:20Z

Where does the actual instrumentation happen? Is there some magic function that does that? cause I don't see that happening 😕

I added two inline comments, do they help? Happy to walk you through the code, too.

evankanderson

You should be aware of:

open-telemetry/opentelemetry-go-contrib#3769

Which is working on generic upstream metrics for http clients. OTOH, it's not clear exactly when that will get merged, so I think this is a reasonable approach in the meantime.

evankanderson · 2023-11-01T17:51:44Z

internal/providers/telemetry/telemetry.go

+	labels := []attribute.KeyValue{
+		attribute.String("http_method", r.Method),
+		attribute.Int("http_status_code", resp.StatusCode),


Do you want to include r.URL.Host as well? I think we'll probably see different latencies from different hosts.

Ah, you're putting it in the metric name instead. That works.

actually, the metric name contains the provider type, not the host. I think your suggestion makes sense.

internal/providers/telemetry/telemetry.go

evankanderson · 2023-11-01T17:55:54Z

internal/providers/telemetry/telemetry.go

+	histogram, ok := m.httpProviderHistograms[providerType]
+	if !ok {
+		var err error
+		histogram, err = m.createProviderHistogram(providerType)
+		if err != nil {
+			return nil, err
+		}
+		m.httpProviderHistograms[providerType] = histogram
+	}
+	return histogram, nil


Do we need to lock this map access? I'm guessing we can get away without it because we're only calling NewDurationRoundTripper at startup?

oh, not yet, but soon we'll need to (PR #1356)

evankanderson · 2023-11-01T17:56:48Z

internal/providers/telemetry/telemetry.go

+
+func (m *httpClientMetrics) getHistogramForProvider(providerType db.ProviderType) (metric.Int64Histogram, error) {
+	histogram, ok := m.httpProviderHistograms[providerType]
+	if !ok {


This can probably be

Suggested change

if !ok {

if histogram == nil {

accepted while making use of xsync.Map

evankanderson · 2023-11-01T18:01:34Z

cmd/dev/app/rule_type/rule_type.go

@@ -130,6 +131,7 @@ func testCmdRun(cmd *cobra.Command, _ []string) error {
 		},
 		db.ProviderAccessToken{},
 		token,
+		provtelemetry.NewNoopMetrics(),


+1 to a functional options pattern.

Does this also mean that the rule type commands actually make calls to providers, or is that just an artifact of how we construct them?

internal/providers/github/github.go

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 ```

jhrozek · 2023-11-01T20:54:34Z

actually, wait up with re-review, I will also use the functional pattern in the server, executor and reconciler constructors.

jhrozek · 2023-11-01T21:18:22Z

actually, wait up with re-review, I will also use the functional pattern in the server, executor and reconciler constructors.

done

evankanderson

This seems fine for now, I don't want to spend too much time noodling how to make this super-clean given our constraints.

evankanderson · 2023-11-01T23:20:17Z

internal/controlplane/handlers_githubwebhooks.go

+	pbOpts := []providers.ProviderBuilderOption{
+		providers.WithProviderMetrics(s.provMt),
+	}
+	provBuilder, err := providers.GetProviderBuilder(ctx, prov, dbRepo.ProjectID, s.store, s.cryptoEngine, pbOpts...)


You don't need to serialize into a slice here (that's the whole point of varargs):

Suggested change

pbOpts := []providers.ProviderBuilderOption{

providers.WithProviderMetrics(s.provMt),

}

provBuilder, err := providers.GetProviderBuilder(ctx, prov, dbRepo.ProjectID, s.store, s.cryptoEngine, pbOpts...)

provBuilder, err := providers.GetProviderBuilder(

ctx, prov, dbRepo.ProjectID, s.store, s.cryptoEngine, providers.WithProviderMetrics(s.provMt))

Yes, this was just a way to not make the line super long (and breaking the line looked visually weird, too). But you're right, I'll reformat the line

You know what, I'll send a follow-up to both suggestions, I want to see the metrics in staging ASAP. Hope it's fine.

evankanderson · 2023-11-01T23:25:47Z

internal/providers/telemetry/telemetry.go

+func (m *httpClientMetrics) NewDurationRoundTripper(
+	wrapped http.RoundTripper,
+	providerType db.ProviderType,
+) (http.RoundTripper, error) {
+	histogram := m.getHistogramForProvider(providerType)


If you want to be really tricky, you can leverage the possibly of a nil client as:

Suggested change

func (m *httpClientMetrics) NewDurationRoundTripper(

wrapped http.RoundTripper,

providerType db.ProviderType,

) (http.RoundTripper, error) {

histogram := m.getHistogramForProvider(providerType)

func (m *httpClientMetrics) NewDurationRoundTripper(

wrapped http.RoundTripper,

providerType db.ProviderType,

) (http.RoundTripper, error) {

if m == nil {

return NewNoopMetrics().NewDurationRoundTripper(wrapped, providerType)

}

histogram := m.getHistogramForProvider(providerType)

jhrozek commented Oct 31, 2023

View reviewed changes

JAORMX reviewed Nov 1, 2023

View reviewed changes

jhrozek commented Nov 1, 2023

View reviewed changes

internal/providers/github/github.go Show resolved Hide resolved

jhrozek commented Nov 1, 2023

View reviewed changes

evankanderson reviewed Nov 1, 2023

View reviewed changes

jhrozek added 4 commits November 1, 2023 20:43

Add r.URL.Host as another label

5401d2e

Use xsync.MapOf

5471f25

functional option pattern for NewProviderBuilder

4748092

jhrozek force-pushed the outbound_api_metrics branch from dd98ee4 to 4748092 Compare November 1, 2023 20:46

Use the functional pattern for server,reconciler and executor, too

84d24d3

evankanderson approved these changes Nov 1, 2023

View reviewed changes

JAORMX approved these changes Nov 2, 2023

View reviewed changes

jhrozek merged commit 81ba458 into mindersec:main Nov 2, 2023
12 checks passed

This was referenced Nov 2, 2023

Remove old token key secrets since we're using Keycloak #1411

Merged

login docs.md #1492

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add metrics for outbound API calls #1360

Add metrics for outbound API calls #1360

jhrozek commented Oct 31, 2023

jhrozek Oct 31, 2023

JAORMX Nov 1, 2023

evankanderson Nov 1, 2023

jhrozek Nov 1, 2023

JAORMX Nov 1, 2023

JAORMX Nov 1, 2023

JAORMX left a comment

jhrozek Nov 1, 2023

jhrozek commented Nov 1, 2023

evankanderson left a comment

evankanderson Nov 1, 2023

evankanderson Nov 1, 2023

jhrozek Nov 1, 2023

evankanderson Nov 1, 2023

jhrozek Nov 1, 2023

jhrozek Nov 1, 2023

evankanderson Nov 1, 2023

jhrozek Nov 1, 2023

evankanderson Nov 1, 2023

jhrozek commented Nov 1, 2023

jhrozek commented Nov 1, 2023

evankanderson left a comment

evankanderson Nov 1, 2023

jhrozek Nov 2, 2023

jhrozek Nov 2, 2023

evankanderson Nov 1, 2023


		s, err := controlplane.NewServer(store, evt, serverMetrics, cfg, vldtr)
		s, err := controlplane.NewServer(store, evt, serverMetrics, providerMetrics, cfg, vldtr)

Add metrics for outbound API calls #1360

Add metrics for outbound API calls #1360

Conversation

jhrozek commented Oct 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JAORMX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhrozek commented Nov 1, 2023

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhrozek commented Nov 1, 2023

jhrozek commented Nov 1, 2023

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment