contrib/k8s.io/client-go: add kubernetes integration, round-tripper #298

dd-caleb · 2018-08-03T20:46:03Z

Fixes #297

I thought that perhaps the easiest way to instrument Kubernetes would be to provide an http.RoundTripper with a couple extra integration points via Options:

	rt := httptrace.WrapRoundTripper(http.DefaultTransport,
		httptrace.WithRoundTripperBefore(func(req *http.Request, span ddtrace.Span) {
			span.SetTag("CalledBefore", true)
		}),
		httptrace.WithRoundTripperAfter(func(res *http.Response, span ddtrace.Span) {
			span.SetTag("CalledAfter", true)
		}))

This would supersede: #172

gbbr

Thanks. Left an initial round of comments

gbbr · 2018-08-06T07:04:43Z

contrib/k8s.io/client-go/kubernetes/kubernetes.go

+)
+
+const (
+	apiPrefix        = "/api/v1/"


Should we do the same here as we did in the other PRs?

const ( prefixApi prefixWatch prefixNamespaces )

Let me know if you have differing thoughts about this approach.

gbbr · 2018-08-06T07:09:04Z

contrib/k8s.io/client-go/kubernetes/kubernetes.go

+	if !strings.HasPrefix(path, apiPrefix) {
+		return resourceName
+	}
+	path = path[len(apiPrefix):]


Let's use path = strings.TrimPrefix(path, prefixAPI). Do you see any problems with that? It's clearer and has the same effect.

gbbr · 2018-08-06T07:09:24Z

contrib/k8s.io/client-go/kubernetes/kubernetes.go

+
+	// strip out /watch
+	if strings.HasPrefix(path, watchPrefix) {
+		path = path[len(watchPrefix):]


Can we use path = strings.TrimPrefix(path, prefixWatch) here too?

gbbr · 2018-08-06T07:15:24Z

contrib/net/http/roundtripper.go

+			err = errors.New(res.Status)
+		}
+	}
+	if rt.cfg.after != nil {


How would you feel about the following change?

We put this in a defer clause right after the before counter-part is called (is this a performance concern?)

We name the error return value of RoundTrip, capture it in the defer and pass it to the after func

The default after func sets the error on the span (we don't set it in Finish).

If people want to skip errors or selectively decide which ones to take into account, they can always make their own "after func"

On the other hand, if this is a performance concern, let's leave it like this (but do set the error by default). If the need arises to work with errors we can just provide a new option to hook into them.

gbbr · 2018-08-06T07:18:24Z

contrib/net/http/roundtripper.go

+	if rt.cfg.after != nil {
+		rt.cfg.after(res, span)
+	}
+	span.Finish(tracer.WithError(err))


Can we defer this too at the top? After the span starts (or in the same defer as the "after func" in you do that).

gbbr · 2018-08-06T07:26:01Z

contrib/net/http/option.go

+		cfg.before = f
+	}
+}
+


Do we need WithServiceName? I realize people can just make a short "before" or "after" func for it but maybe it would be nice to simplify this and also keep it matching our other contribs.

WithServiceName already exists for the MuxOption

I can't easily figure out how MuxOption connects to WrapRoundTripper, which uses RoundTripperOption. There is no service set in RoundTrip currently, it will simply use whatever was set on the tracer. If nothing is set, the span will be refused by the agent. Am I missing something?

I can't create a WithServiceName function for RoundTripperOption because it's already used by the MuxOption which is in the same package. Should I create a WithRoundTripperServiceName function?

I'm not sure. Would UseServiceName be bad?

gbbr · 2018-08-06T07:30:22Z

contrib/net/http/roundtripper_test.go

+	assert.Len(t, spans, 2)
+	assert.Equal(t, spans[0].TraceID(), spans[1].TraceID())
+
+	{


I saw we've started using this pattern. I see it a bit as a "lazy mode to create a closure". Maybe I'm not looking at it the right way? I'm wondering if we can't just use a for loop and sub-tests?

Not sure I follow about the for loop... the things being tested for aren't the same for each span.

I used a block to re-use the s variable. I can change it to s1, s2.

I'd prefer it without the closure if possible. But I am open to considering it. I've been generally avoiding using it.

gbbr · 2018-08-06T07:49:34Z

contrib/net/http/roundtripper.go

+
+func (rt *roundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
+	span, _ := tracer.StartSpanFromContext(req.Context(), "http.request",
+		tracer.ResourceName(defaultResourceName),


We're going to need to set the right type for the rest of our pipeline, this is an important part:

tracer.SpanType(ext.SpanTypeHTTP)

When the span type is set on a root span, it classifies the service in the application and enables making use of other systems such as obfuscation, quantization, etc.

gbbr · 2018-08-06T07:54:58Z

contrib/k8s.io/client-go/kubernetes/kubernetes.go

+	// {type}/{name}
+	lastType := ""
+	for i := 0; ; i++ {
+		idx := strings.IndexByte(path, '/')


This solution is very low level. Can we make use of the strings package more? For example:

parts := strings.Split(path, "/") // n := len(parts) for i, p := range parts {

gbbr · 2018-08-06T07:57:34Z

contrib/k8s.io/client-go/kubernetes/kubernetes.go

+}
+
+func requestToResource(method, path string) string {
+	resourceName := method + " "


Instead of repeatedly concatenating string constants, can we use strings.Builder here?

if !strings.HasPrefix(path, prefixApi) { return method } var out strings.Builder out.WriteString(method) out.WriteByte(' ') // ... return out.String()

gbbr · 2018-08-06T08:12:04Z

Fixes #172, right?

gbbr · 2018-08-06T08:15:09Z

This is basically 2 PRs, do you think we could split it? The reason is that it turns out to be very helpful to do that when putting together a release change log. Each commit message is a clear one line description of the affected package and the change, as well as a link to a PR. This makes it really easy to cover everything when releasing. It also makes it easier to not miss anything when navigating commit history.

I'd also be fine with two commits, except that would create an extra merge commit, but I'd be happy with that compromise if you prefer this alternative.

…atch upstream changes, use a string builder for resource naming, refactor loop to use Split instead of IndexByte

dd-caleb · 2018-08-08T13:48:22Z

@gbbr The http changes were split out and merged to v1. I think this PR should be ready to merge. Let me know if you see any additional problems. Thanks.

gbbr · 2018-08-07T07:54:59Z

contrib/k8s.io/client-go/kubernetes/kubernetes.go

+
+	path = strings.TrimPrefix(path, prefixAPI)
+
+	// strip out /watch


Can we put this comment inside the if statement? That's what I've been doing all around. It makes sense (to me) because that's where the action happens (inside the if statement).

gbbr · 2018-08-07T07:55:17Z

contrib/k8s.io/client-go/kubernetes/kubernetes.go

+	}
+
+	// {type}/{name}
+	lastType := ""


For "zero values" can we use?

var lastType string

dd-caleb added apm:ecosystem contrib/* related feature requests or bugs feature-request labels Aug 3, 2018

gbbr reviewed Aug 6, 2018

View reviewed changes

gbbr added this to the next milestone Aug 6, 2018

contrib/net/http: add tracing for http.RoundTripper

71e5f1b

dd-caleb mentioned this pull request Aug 6, 2018

contrib/net/http: add tracing for http.RoundTripper #299

Merged

dd-caleb added 2 commits August 6, 2018 09:20

contrib/k8s.io/client-go: add kubernetes integration, round-tripper

18d4d3a

contrib/k8s.io/client-go: rename prefix constants, update option to m…

922d575

…atch upstream changes, use a string builder for resource naming, refactor loop to use Split instead of IndexByte

dd-caleb force-pushed the caleb/kubernetes branch from 426a579 to 922d575 Compare August 6, 2018 13:30

Merge branch 'v1' into caleb/kubernetes

3bfd509

gbbr approved these changes Aug 8, 2018

View reviewed changes

contrib/k8s.io/client-go: move comment, use zero value for string

41e0362

dd-caleb merged commit 7599e40 into v1 Aug 8, 2018

dd-caleb deleted the caleb/kubernetes branch August 8, 2018 14:08

gbbr modified the milestones: next, 1.2.0 Aug 22, 2018

mingrammer pushed a commit to mingrammer/dd-trace-go that referenced this pull request Dec 22, 2020

contrib/k8s.io/client-go: add kubernetes integration (DataDog#298)

35818f3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contrib/k8s.io/client-go: add kubernetes integration, round-tripper #298

contrib/k8s.io/client-go: add kubernetes integration, round-tripper #298

dd-caleb commented Aug 3, 2018

gbbr left a comment

gbbr Aug 6, 2018

gbbr Aug 6, 2018

gbbr Aug 6, 2018

gbbr Aug 6, 2018

gbbr Aug 6, 2018

gbbr Aug 6, 2018

dd-caleb Aug 6, 2018

gbbr Aug 6, 2018

dd-caleb Aug 6, 2018

gbbr Aug 6, 2018

gbbr Aug 6, 2018

dd-caleb Aug 6, 2018

gbbr Aug 6, 2018

gbbr Aug 6, 2018

gbbr Aug 6, 2018

gbbr Aug 6, 2018 •

edited

Loading

gbbr commented Aug 6, 2018

gbbr commented Aug 6, 2018 •

edited

Loading

dd-caleb commented Aug 8, 2018

gbbr Aug 7, 2018

gbbr Aug 7, 2018


		path = strings.TrimPrefix(path, prefixAPI)

		// strip out /watch

contrib/k8s.io/client-go: add kubernetes integration, round-tripper #298

contrib/k8s.io/client-go: add kubernetes integration, round-tripper #298

Conversation

dd-caleb commented Aug 3, 2018

gbbr 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

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

gbbr Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

gbbr commented Aug 6, 2018

gbbr commented Aug 6, 2018 • edited Loading

dd-caleb commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbbr Aug 6, 2018 •

edited

Loading

gbbr commented Aug 6, 2018 •

edited

Loading