[WIP] Pod tracing #3491

greghaynes · 2019-03-21T20:04:11Z

When performing cold start the majority of our time is spent waiting for
our pod to be created. Unfortunately, because we do not control anything
beyond talking to kube-api, we have to perform our own resource tracking
to create spans which tie in to our existing cold-start tracing.

This depends on #2726 and #3881

Release Note

NONE

knative-prow-robot

@greghaynes: 20 warnings.

In response to this:

Release Note
NONE

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.

pkg/tracing/config/tracing.go

knative-prow-robot · 2019-03-21T20:04:26Z

pkg/tracing/config/tracing.go

+	if enable, ok := cfgMap[enableKey]; ok {
+		if enableBool, err := strconv.ParseBool(enable); err != nil {
+			return nil, fmt.Errorf("Failed parsing tracing config %q: %v", enableKey, err)
+		} else {


Golint indent: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary). More info.

knative-prow-robot · 2019-03-21T20:04:26Z

pkg/tracing/config/tracing.go

+	if debug, ok := cfgMap[debugKey]; ok {
+		if debugBool, err := strconv.ParseBool(debug); err != nil {
+			return nil, fmt.Errorf("Failed parsing tracing config %q", debugKey)
+		} else {


Golint indent: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary). More info.

knative-prow-robot · 2019-03-21T20:04:28Z

pkg/tracing/tracer.go

+	}
+}
+
+func CreateTracer(cfg *config.Config, reporter zipkinreporter.Reporter, serviceName, hostPort string) (*zipkin.Tracer, error) {


Golint comments: exported function CreateTracer should have comment or be unexported. More info.

knative-prow-robot · 2019-03-21T20:04:28Z

pkg/tracing/http.go

+	"go.uber.org/zap"
+)
+
+type TracerRefGetter func(context.Context) (*TracerRef, error)


Golint comments: exported type TracerRefGetter should have comment or be unexported. More info.

knative-prow-robot · 2019-03-21T20:04:28Z

pkg/tracing/cache.go

+	}, nil
+}
+
+type ReporterFactory func(cfg *config.Config) (zipkinreporter.Reporter, error)


Golint comments: exported type ReporterFactory should have comment or be unexported. More info.

knative-prow-robot · 2019-03-21T20:04:28Z

pkg/tracing/cache.go

+	reporterRef *refcountedReporter
+}
+
+func (tr *TracerRef) Ref() {


Golint comments: exported method TracerRef.Ref should have comment or be unexported. More info.

knative-prow-robot · 2019-03-21T20:04:28Z

pkg/tracing/cache.go

+	tr.reporterRef.refs.RLock()
+}
+
+func (tr *TracerRef) Done() {


Golint comments: exported method TracerRef.Done should have comment or be unexported. More info.

knative-prow-robot · 2019-04-24T21:32:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: greghaynes
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mattmoor

If they are not already assigned, you can assign the PR to them by writing /assign @mattmoor in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

This span clearly shows time spent waiting for our endpoint to become ready

During cold start it is very useful to see spans on how long the k8s resources we wait for (pods) take in various states relative to our cold-start request.

knative-metrics-robot · 2019-05-15T17:30:54Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/tracing/config/tracing.go	82.6%	80.0%	-2.6
pkg/tracing/pod_tracer.go	Do not exist	100.0%

knative-prow-robot · 2019-05-15T17:35:13Z

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

Test name	Commit	Details	Rerun command
pull-knative-serving-unit-tests	`c17b16f`	link	`/test pull-knative-serving-unit-tests`
pull-knative-serving-build-tests	`c17b16f`	link	`/test pull-knative-serving-build-tests`

Full PR test history. Your PR dashboard.

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.

markusthoemmes · 2019-07-09T08:34:09Z

This seems vastly out of date. Any status @greghaynes?

greghaynes · 2019-07-09T16:32:56Z

I've gotten a lot of mileage out of this PR which is why I've left it (lots of testing features / fixes using this). The reason it has remained WIP though is I am not sure how to do this in a way where it isn't very fragile. Right now we are manually finding a pod and tracing it. If a pod is scaling down while the request comes in to scale up from 0 it is difficult to pick the right pod. Theres also some potential races where requests come in to activator but the pod is already scaled up.

My main question is - if I were to write some more code to pick the correct pod and handle this race, is this something we'd want to maintain? I have been sort of on the fence about this - its optional code and I'd worry itll be rarely run and has a somewhat complex state machine...

jonjohnsonjr · 2019-07-09T18:08:23Z

markusthoemmes · 2019-08-07T14:29:45Z

Is this approach valid at this point? We're running however many requests through the activator as we want and even potentially always keep it in path. It feels like this is generating a lot of overhead and the traces cannot be mapped one to one to the requests anyway.

So I think the races you mentioned are amplified at this point. I'm not sure how we'd go about making this as robust as we'd like it to be 🤔. Is this still producing very vital information for us?

markusthoemmes · 2019-10-28T08:36:09Z

I'm inclined to close this. It's vastly outdated and if we wanted to adopt it, I think we'd need to do some designwork around this.

nimakaviani · 2019-10-28T14:26:51Z

I have done a rebase fairly recently that I have been using for some performance assessment: https://github.com/nimakaviani/serving/tree/pod-tracing-redux

But @greghaynes 's points on the complex state machine and whether or not we want to maintain it still hold. Lets decide on its faith in the next WG meeting. I add it to the agenda.

markusthoemmes · 2019-11-28T08:24:07Z

Going to close this as @nimakaviani seems to have a working version himself. Feel free to reopen once we determined the future of this?

/close

knative-prow-robot · 2019-11-28T08:24:09Z

@markusthoemmes: Closed this PR.

In response to this:

Going to close this as @nimakaviani seems to have a working version himself. Feel free to reopen once we determined the future of this?

/close

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.

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 21, 2019

knative-prow-robot requested review from ericKlawitter, lichuqiang, mdemirhan and tcnghia March 21, 2019 20:04

knative-prow-robot added area/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Mar 21, 2019

knative-prow-robot reviewed Mar 21, 2019

View reviewed changes

mattmoor-sockpuppet reviewed Mar 21, 2019

View reviewed changes

greghaynes changed the title ~~Pod tracing~~ [WIP] Pod tracing Mar 21, 2019

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 21, 2019

greghaynes force-pushed the pod-tracing branch from deeaaf3 to 52d5e39 Compare March 21, 2019 21:57

mattmoor-sockpuppet reviewed Mar 21, 2019

View reviewed changes

greghaynes force-pushed the pod-tracing branch from 52d5e39 to 45ea70b Compare March 25, 2019 15:48

mattmoor-sockpuppet reviewed Mar 25, 2019

View reviewed changes

greghaynes force-pushed the pod-tracing branch from 45ea70b to f31e794 Compare March 25, 2019 22:47

mattmoor-sockpuppet reviewed Mar 25, 2019

View reviewed changes

greghaynes force-pushed the pod-tracing branch 2 times, most recently from d09b9e2 to 7eef4d1 Compare March 28, 2019 22:52

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 28, 2019

mattmoor-sockpuppet reviewed Mar 28, 2019

View reviewed changes

greghaynes force-pushed the pod-tracing branch from 7eef4d1 to 1f973bd Compare April 2, 2019 22:22

mattmoor-sockpuppet reviewed Apr 2, 2019

View reviewed changes

greghaynes force-pushed the pod-tracing branch from 1f973bd to db1fe37 Compare April 2, 2019 22:40

mattmoor-sockpuppet reviewed Apr 2, 2019

View reviewed changes

greghaynes force-pushed the pod-tracing branch from db1fe37 to dc5a290 Compare April 3, 2019 16:10

mattmoor-sockpuppet reviewed Apr 3, 2019

View reviewed changes

greghaynes mentioned this pull request Apr 15, 2019

[WIP] Activator daemonset #2557

Closed

greghaynes force-pushed the pod-tracing branch from dc5a290 to 06412a5 Compare April 24, 2019 21:32

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/monitoring and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 24, 2019

greghaynes force-pushed the pod-tracing branch 2 times, most recently from 06f7e61 to d90bb9a Compare April 24, 2019 21:37

Gregory Haynes added 2 commits May 15, 2019 09:38

Add endpoint_wait span in activator

a301826

This span clearly shows time spent waiting for our endpoint to become ready

Support tracing pods during cold-start

c17b16f

During cold start it is very useful to see spans on how long the k8s resources we wait for (pods) take in various states relative to our cold-start request.

greghaynes force-pushed the pod-tracing branch from d90bb9a to c17b16f Compare May 15, 2019 17:27

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 15, 2019

greghaynes mentioned this pull request Jun 3, 2019

use exec probe for queue proxy readiness check #4148

Merged

knative-prow-robot closed this Nov 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Pod tracing #3491

[WIP] Pod tracing #3491

greghaynes commented Mar 21, 2019 •

edited

Loading

knative-prow-robot left a comment

knative-prow-robot Mar 21, 2019

knative-prow-robot Mar 21, 2019

knative-prow-robot Mar 21, 2019

knative-prow-robot Mar 21, 2019

knative-prow-robot Mar 21, 2019

knative-prow-robot Mar 21, 2019

knative-prow-robot Mar 21, 2019

knative-prow-robot commented Apr 24, 2019

knative-metrics-robot commented May 15, 2019

knative-prow-robot commented May 15, 2019

markusthoemmes commented Jul 9, 2019

greghaynes commented Jul 9, 2019

jonjohnsonjr commented Jul 9, 2019

markusthoemmes commented Aug 7, 2019

markusthoemmes commented Oct 28, 2019

nimakaviani commented Oct 28, 2019

markusthoemmes commented Nov 28, 2019

knative-prow-robot commented Nov 28, 2019

[WIP] Pod tracing #3491

[WIP] Pod tracing #3491

Conversation

greghaynes commented Mar 21, 2019 • edited Loading

knative-prow-robot left a comment

Choose a reason for hiding this comment

knative-prow-robot Mar 21, 2019

Choose a reason for hiding this comment

knative-prow-robot Mar 21, 2019

Choose a reason for hiding this comment

knative-prow-robot Mar 21, 2019

Choose a reason for hiding this comment

knative-prow-robot Mar 21, 2019

Choose a reason for hiding this comment

knative-prow-robot Mar 21, 2019

Choose a reason for hiding this comment

knative-prow-robot Mar 21, 2019

Choose a reason for hiding this comment

knative-prow-robot Mar 21, 2019

Choose a reason for hiding this comment

knative-prow-robot commented Apr 24, 2019

knative-metrics-robot commented May 15, 2019

knative-prow-robot commented May 15, 2019

markusthoemmes commented Jul 9, 2019

greghaynes commented Jul 9, 2019

jonjohnsonjr commented Jul 9, 2019

markusthoemmes commented Aug 7, 2019

markusthoemmes commented Oct 28, 2019

nimakaviani commented Oct 28, 2019

markusthoemmes commented Nov 28, 2019

knative-prow-robot commented Nov 28, 2019

greghaynes commented Mar 21, 2019 •

edited

Loading