WIP: Simple instrumentation with Jaeger backend #45962

gmarek · 2017-05-17T13:44:17Z

This adds OpenTracing instrumentation to the apiserver and client-go. It does all instrumentation work and only thing that's left to allow users using client-go instrumentation is allowing passing context.Context to client calls (@caesarxuchao @kubernetes/sig-api-machinery-feature-requests). Once this is done we'll need to write a short doc describing how to use this.

cc @yurishkuro @bhs

Add OpenTracing instrumentation to client-go and apiserver.

k8s-github-robot · 2017-05-17T13:44:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gmarek
We suggest the following additional approver: bgrant0607

Assign the PR to them by writing /assign @bgrant0607 in a comment when ready.

Needs approval from an approver in each of these OWNERS Files:

OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

gmarek · 2017-05-25T17:56:59Z

Only minor godep-related fixes are needed, I'll apply them tomorrow.

gmarek · 2017-05-26T11:37:43Z

@k8s-bot node e2e test this

yurishkuro · 2017-05-26T22:26:13Z

staging/src/k8s.io/client-go/rest/request.go

+	if r.ctx == nil {
+		r.ctx = context.Background()
+	}
+	span, ctx := opentracing.StartSpanFromContext(r.ctx, fmt.Sprintf("%v %v", r.verb, r.resource))


note: this will create a bare bones span with little useful information. You may want to consider using https://github.com/opentracing-contrib/go-stdlib/ (or replicating that logic if you can do it more efficiently internally).

It'd be way easier if there was a nethttp.NewFromTracer(tr *opentracing.Tracer, opts...) Tracer function in the library... Currently I don't think it's worth the effort.

if there was a nethttp.NewFromTracer(tr *opentracing.Tracer, opts...)

that's fair, my point is that a bare-bones span is not very useful when you actually want to troubleshoot some traces

Do you think you can add such a function to the library?

I had a look, the existing code there is too coupled. I think it's simpler to add the tags explicitly, rather than pull another dependency just for that. You want at least these:

ext.HTTPMethod.Set(span, req.Method) ext.HTTPUrl.Set(span, req.URL.String()) ext.SpanKindRPCClient.Set(span) ext.Component.Set(span, componentName) // optional

If you have the peer/callee address easily accessible, it's also useful to set

ext.PeerAddress.Set(span, address) // IP string or anything, really

bhs

Main comment: this is a giant PR with the vendored code, etc... can you point me to the parts you are most concerned about / want feedback about from an OT standpoint?

I've made one tiny comment in the meantime. :)

Thanks!

bhs · 2017-05-30T16:29:31Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go

+}
+
+func startTracing(req *http.Request, operationName string) (otSpan opentracing.Span, simpleTrace *utiltrace.Trace) {
+	trace := utiltrace.New(operationName)


I'm sure you considered it, but why not add an opentracing.Span field to utiltrace.Trace? Then you wouldn't need to pass them around together.

I wanted to get rid of the old trace library, and I figured that this was was easier.

gmarek · 2017-05-30T16:40:29Z

@bhs - only first three commits matter. Rest is generated code and godeps.

caesarxuchao · 2017-05-30T17:54:41Z

@gmarek is this PR targeting 1.7?

gmarek · 2017-05-30T19:00:27Z

No, I don't think it is.

yurishkuro · 2017-05-31T16:34:42Z

staging/src/k8s.io/client-go/rest/request.go

+		span := opentracing.SpanFromContext(r.ctx)
+		if span != nil {
+			ext.HTTPMethod.Set(span, req.Method)
+			ext.HTTPUrl.Set(span, req.URL.String())


nit: reuse url from above

yurishkuro · 2017-05-31T16:37:41Z

staging/src/k8s.io/client-go/rest/request.go

+				span.Context(),
+				opentracing.HTTPHeaders,
+				opentracing.HTTPHeadersCarrier(req.Header))
+		}

 		r.backoffMgr.Sleep(r.backoffMgr.CalculateBackoff(r.URL()))
 		if retries > 0 {


what will happen if there are retries? The span seems to be created earlier, in the caller.

timothysc · 2017-06-04T21:29:08Z

I saw this come in, but I'm prioritizing 1.7 items atm.

k8s-ci-robot · 2017-06-12T09:28:45Z

@gmarek: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-federation-e2e-gce	ba13d1385f9b48d9abd9198f0458bceb0a5cb956	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`
pull-kubernetes-bazel	`fa003ad`	link	`@k8s-bot pull-kubernetes-bazel test this`
pull-kubernetes-verify	`fa003ad`	link	`@k8s-bot pull-kubernetes-verify test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-github-robot · 2017-06-21T22:19:20Z

@gmarek PR needs rebase

timothysc · 2017-07-18T21:45:15Z

I'm for dropping in favor of updated Events proposal.

bhs · 2017-07-19T15:44:42Z

@timothysc @gmarek for those of us who are not k8s insiders, can you provide a pointer to the "Events" proposal? thanks.

gmarek · 2017-07-19T15:46:50Z

Sure, sorry - https://docs.google.com/a/google.com/document/d/13BeJlrEcJhSKgsHOHWmHdJGqXrjSm_o9XxOtwcN6yNg/edit?usp=sharing

I also wanted to finish this work (there's not a lot to do), so we have tracing inside API server, but I didn't have time:(

gmarek added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label May 17, 2017

gmarek self-assigned this May 17, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 17, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels May 17, 2017

gmarek force-pushed the jaeger branch from 8437cc7 to b5a3554 Compare May 19, 2017 07:35

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2017

gmarek force-pushed the jaeger branch from b5a3554 to dedd102 Compare May 25, 2017 10:24

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2017

gmarek force-pushed the jaeger branch from dedd102 to e1983cc Compare May 25, 2017 12:17

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label May 25, 2017

gmarek force-pushed the jaeger branch 2 times, most recently from 9b81105 to c0e347e Compare May 25, 2017 12:56

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 25, 2017

gmarek force-pushed the jaeger branch 3 times, most recently from fca9cee to c1c8315 Compare May 25, 2017 17:07

k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 25, 2017

gmarek force-pushed the jaeger branch 2 times, most recently from b9da01f to 859c350 Compare May 26, 2017 10:31

gmarek assigned wojtek-t and timothysc and unassigned gmarek May 26, 2017

gmarek mentioned this pull request May 26, 2017

It should be possible to inject Context into client-go API call #46503

Closed

yurishkuro reviewed May 26, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2017

gmarek force-pushed the jaeger branch from 859c350 to 2382b2b Compare May 30, 2017 09:48

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2017

gmarek force-pushed the jaeger branch 2 times, most recently from db4f638 to ce6f6dc Compare May 30, 2017 11:20

bhs reviewed May 30, 2017

View reviewed changes

gmarek force-pushed the jaeger branch from ce6f6dc to ba13d13 Compare May 31, 2017 16:24

yurishkuro reviewed May 31, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 2, 2017

timothysc added this to the v1.8 milestone Jun 4, 2017

gmarek added 5 commits June 12, 2017 10:32

Add basic Jaeger tracer to master components

778b61b

Add OpenTracing instrumentation to the api server

0eb8ab3

Add OpenTracing instrumentation to client-go

36063a4

generated

f66c401

godeps

fa003ad

gmarek force-pushed the jaeger branch from ba13d13 to fa003ad Compare June 12, 2017 08:36

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 12, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 21, 2017

gmarek closed this Jul 19, 2017

gmarek mentioned this pull request Aug 17, 2017

Consider adding tracing support for OpenTracing (or similar) #26507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Simple instrumentation with Jaeger backend #45962

WIP: Simple instrumentation with Jaeger backend #45962

gmarek commented May 17, 2017 •

edited

k8s-github-robot commented May 17, 2017

gmarek commented May 25, 2017

gmarek commented May 26, 2017

yurishkuro May 26, 2017

gmarek May 30, 2017

yurishkuro May 30, 2017

gmarek May 30, 2017

yurishkuro May 30, 2017

gmarek May 31, 2017

bhs left a comment

bhs May 30, 2017

gmarek May 30, 2017

gmarek commented May 30, 2017

caesarxuchao commented May 30, 2017

gmarek commented May 30, 2017

yurishkuro May 31, 2017

yurishkuro May 31, 2017

timothysc commented Jun 4, 2017

k8s-ci-robot commented Jun 12, 2017 •

edited

k8s-github-robot commented Jun 21, 2017

timothysc commented Jul 18, 2017

bhs commented Jul 19, 2017

gmarek commented Jul 19, 2017

WIP: Simple instrumentation with Jaeger backend #45962

WIP: Simple instrumentation with Jaeger backend #45962

Conversation

gmarek commented May 17, 2017 • edited

k8s-github-robot commented May 17, 2017

gmarek commented May 25, 2017

gmarek commented May 26, 2017

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

bhs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarek commented May 30, 2017

caesarxuchao commented May 30, 2017

gmarek commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothysc commented Jun 4, 2017

k8s-ci-robot commented Jun 12, 2017 • edited

k8s-github-robot commented Jun 21, 2017

timothysc commented Jul 18, 2017

bhs commented Jul 19, 2017

gmarek commented Jul 19, 2017

gmarek commented May 17, 2017 •

edited

k8s-ci-robot commented Jun 12, 2017 •

edited