Tracing KEP: Move to Implementable #1458

dashpole · 2020-01-16T02:01:39Z

cc @kubernetes/sig-api-machinery-misc @lavalamp @logicalhan @deads2k @liggitt @mikedanese

dashpole · 2020-03-16T17:25:26Z

I decided to repurpose this PR as the PR to move the KEP to implementable, and will include other changes to address feedback in it.

cc @kubernetes/sig-instrumentation-api-reviews

dashpole · 2020-03-16T17:26:17Z

/assign @brancz

brancz · 2020-03-24T09:20:38Z

Any particular reason this is [WIP]?

dashpole · 2020-03-24T16:45:36Z

Removed the WIP tag. This is really a
/hold
for discussion on OT Collector.

keps/sig-instrumentation/0034-distributed-tracing-kep.md

serathius · 2020-03-24T19:16:38Z

I think we should follow similar versioning conversion like other annotations e.g. "alpha.trace.kubernetes.io/context"

keps/sig-instrumentation/0034-distributed-tracing-kep.md

dashpole · 2020-03-28T00:14:45Z

I think we should follow similar versioning conversion like other annotations e.g. "alpha.trace.kubernetes.io/context"

Makes sense. I'm going to wait until the discussion about updating annotations with subresource update is completed before updating.

keps/sig-instrumentation/0034-distributed-tracing-kep.md

dashpole · 2020-10-01T19:51:09Z

@BenTheElder can you take a pass on the testing paragraph? It should be straight-forward, but it would be good to get someone's eyes from sig-testing on it.

kikisdeliveryservice

If this is for #647 then the dir should be: 647-apiserver-tracing

KEP-647, etc...

As we track via the issue number and issue 34 that you are referencing seems to be something... completely different. :)

dashpole · 2020-10-08T16:53:59Z

@kikisdeliveryservice I updated the KEP number. The KEP was created when KEP numbers were based on a next kep number file...

keps/sig-instrumentation/647-apiserver-tracing/README.md

lavalamp · 2020-10-08T20:29:59Z

keps/sig-instrumentation/647-apiserver-tracing/README.md

+
+### Test Plan
+
+We will e2e test this feature by deploying an OpenTelemetry Collector on the master, and configure it to export traces using the [stdout exporter](https://github.com/open-telemetry/opentelemetry-go/tree/master/exporters/stdout), which logs the spans in json format.  We can then verify that the logs contain our expected traces when making calls to the API Server.


Do you plan this to ever be conformance tested? This doesn't sound easy to conformance test.

Yes, it would be good to have this conformance tested. I added it as a GA requirement. With the current configuration options, we can run the OTel Collector as a service in the cluster instead of on the master node.

The thing is, users likely won't be able to configure that config file. But yeah, I agree with testing via running this in the cluster.

lavalamp · 2020-10-08T20:30:43Z

keps/sig-instrumentation/647-apiserver-tracing/README.md

+## Production Readiness Survey
+
+* Feature enablement and rollback
+  - How can this feature be enabled / disabled in a live cluster?  **Feature-gate: APIServerTracing.  The API Server must be restarted to enable/disable exporting spans.**


Once the feature is GA and we remove the feature gate, how will one turn it off if it's not desired?

I specified that if the configuration file path isn't specified, tracing is disabled. Alternatively, we can add a specific configuration option in the config to specify if it is disabled.

WDYT?

Yeah going off of the config file presence is fine.

keps/sig-instrumentation/647-apiserver-tracing/README.md

lavalamp · 2020-10-08T20:35:19Z

keps/sig-instrumentation/647-apiserver-tracing/README.md

+
+We will wrap the API Server's http server and http clients with [otelhttp](https://github.com/open-telemetry/opentelemetry-go-contrib/tree/master/instrumentation/net/http/otelhttp) to get spans for incoming and outgoing http requests.  This generates spans for all sampled incoming requests and propagates context with all client requests.  For incoming requests, this would go below [WithRequestInfo](https://github.com/kubernetes/kubernetes/blob/9eb097c4b07ea59c674a69e19c1519f0d10f2fa8/staging/src/k8s.io/apiserver/pkg/server/config.go#L676) in the filter stack, as it must be after authentication and authorization, before the panic filter, and is closest in function to the WithRequestInfo filter.
+
+Note that some clients of the API Server, such as webhooks, may make reentrant calls to the API Server.  To gain the full benefit of tracing, such clients should propagate context with requests back to the API Server.


Please expand this, how do clients do that?

I have an extremely strong interest in apiserver knowing when an incoming API call is re-entrant, and ideally, what its parent call is. Additionally, I'd like this to work in a multiple apiserver setup. So:

Can the tracing header be reverse-engineered to figure this out?

Can I pack metadata into the tracing header to make this possible?

I gave an example of how a webhook would do that. The tracing header will tell you if it is a direct child of the API Server trace, but you can't tell if it is an indirect child (e.g. if the admission controller webhook adds a span in-between). You can pack metadata into the tracing header (well, it is a different header, but still), using "Baggage": https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/baggage/api.md#baggage-api.

Thanks, love the name "Baggage"

lavalamp · 2020-10-08T20:39:40Z

keps/sig-instrumentation/647-apiserver-tracing/README.md

+
+This KEP proposes the use of the [OpenTelemetry tracing framework](https://opentelemetry.io/) to create and export spans to configured backends.
+
+The API Server will use the [OpenTelemetry exporter format](https://github.com/open-telemetry/opentelemetry-proto), and the [OTlp exporter](https://github.com/open-telemetry/opentelemetry-go/tree/master/exporters/otlp#opentelemetry-collector-go-exporter) which can export traces.  This format is easy to use with the [OpenTelemetry Collector](https://github.com/open-telemetry/opentelemetry-collector), which allows importing and configuring exporters for trace storage backends to be done out-of-tree in addition to other useful features.


I think I'm OK for approving this for alpha; for beta I think @deads2k should also approve and at least one of us actually needs to look at the OT code, I haven't...

Got it. I added a "revisit format" item for beta.

evankanderson · 2020-10-08T21:50:35Z

I just noticed this and the Structured Logging KEP.

I see this is listed as a non-goal:

Change metrics or logging (e.g. to support trace-metric correlation)

Having recently worked on observability for Knative, I discovered that having a common set of labels across all the observability components (metrics, logs, tracing) helped with both reducing boilerplate and increasing accuracy/coverage of labels. It's also really painful to align these after you've done a bunch of instrumentation work for each separately and downstream teams have taken dependencies on the specific labels.

(The interface I'm looking at extracts a logger from the context, and that logger can be augmented with trace labels before it is returned.)

It looks like KEP 1602 doesn't define many well-known keys (ts, v, err, msg), but the implementation (e.g. this PR https://github.com/kubernetes/kubernetes/pull/91576/files) seems to have a bunch more labels that might apply to tracing, including object, kind, objectUID, as well as some kind-specific strings such as service, node, and deployment.

https://github.com/kubernetes/kubernetes/pull/91773/files#diff-06ce45a2e81bbfd5d4ce7e3c67d49a84R47 also seems to introduce/use logTraceArgs that seems related.

evankanderson · 2020-10-08T21:51:25Z

(Don't consider my comment blocking, it's a drive-by because I was commenting on my experience and someone upstream pointed out this KEP.)

dashpole · 2020-10-08T22:26:41Z

@evankanderson thanks for the feedback; you make good points. I definitely agree with the value of having the same set of labels. One problem right now is that we have two different standards: the long-standing kubernetes instrumentation guidelines and the OpenTelemetry semantic conventions. The kubernetes standards suggest using "pod", and the OpenTelemetry standards suggest "k8s.pod", for example. I don't have a good answer, but I'll bring it up for discussion at next week's instrumentation meeting.

lavalamp · 2020-10-08T22:26:41Z

/lgtm
/approve

k8s-ci-robot · 2020-10-08T22:27:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, lavalamp

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:

~~keps/sig-instrumentation/OWNERS~~ [dashpole]

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

dashpole · 2020-10-08T22:27:40Z

/hold cancel

lavalamp · 2020-10-08T22:28:13Z

I think we'll want to encourage clients as strongly as possible to pass those headers through.

Is it easy to describe in non-go terms what a client has to do? Hopefully just copy-paste some headers matching a particular name format?

dashpole · 2020-10-08T22:35:22Z

Is it easy to describe in non-go terms what a client has to do? Hopefully just copy-paste some headers matching a particular name format?

Yes. This describes the trace context header: https://www.w3.org/TR/trace-context/. And this describes the baggage header: https://w3c.github.io/baggage/. So you would have to copy the traceparent, baggage, and tracestate headers.

But in general, OpenTelemetry has broad language support, which should be easier than manually copying headers.

k8s-ci-robot requested review from bgrant0607 and piosz January 16, 2020 02:02

dashpole changed the title ~~Tracing KEP: Address feedback from sig-apimachinery~~ [WIP] Tracing KEP: Move to Implementable Mar 16, 2020

k8s-ci-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 16, 2020

dashpole force-pushed the tracing_updates branch from 3603ce5 to 44f1d5e Compare March 16, 2020 17:18

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 16, 2020

dashpole force-pushed the tracing_updates branch from 44f1d5e to 259e3f3 Compare March 16, 2020 17:24

k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Mar 16, 2020

k8s-ci-robot assigned brancz Mar 16, 2020

dashpole changed the title ~~[WIP] Tracing KEP: Move to Implementable~~ Tracing KEP: Move to Implementable Mar 24, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 24, 2020

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 24, 2020

serathius reviewed Mar 24, 2020

View reviewed changes

keps/sig-instrumentation/0034-distributed-tracing-kep.md Outdated Show resolved Hide resolved

bgrant0607 removed their request for review March 24, 2020 19:14

yue9944882 reviewed Mar 25, 2020

View reviewed changes

keps/sig-instrumentation/0034-distributed-tracing-kep.md Outdated Show resolved Hide resolved

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 4, 2020

dashpole commented Apr 4, 2020

View reviewed changes

keps/sig-instrumentation/0034-distributed-tracing-kep.md Outdated Show resolved Hide resolved

dashpole added 9 commits October 1, 2020 12:47

simplify proposal to only trace api server requests

171a003

clarify additional request latency

efff508

batching and filter details

53f3446

add details on egress with client

c9289ad

remove kubectl section

156fb60

add specific configuration options

2fca880

switch to file-based configuration

c35f925

formatting

82addef

update to new format, and add test plan

e289df7

dashpole force-pushed the tracing_updates branch from adb386e to e289df7 Compare October 1, 2020 19:48

kikisdeliveryservice requested changes Oct 8, 2020

View reviewed changes

change kep number from 0034 to 647

0fb51ea

lavalamp reviewed Oct 8, 2020

View reviewed changes

address comments

088ca96

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 8, 2020

k8s-ci-robot merged commit 7340d5b into kubernetes:master Oct 8, 2020

k8s-ci-robot added this to the v1.20 milestone Oct 8, 2020

dashpole deleted the tracing_updates branch October 8, 2020 22:29

bboreham mentioned this pull request Oct 9, 2020

🌱 Add distributed tracing framework kubernetes-sigs/controller-runtime#1211

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tracing KEP: Move to Implementable #1458

Tracing KEP: Move to Implementable #1458

dashpole commented Jan 16, 2020

dashpole commented Mar 16, 2020

dashpole commented Mar 16, 2020

brancz commented Mar 24, 2020

dashpole commented Mar 24, 2020

serathius commented Mar 24, 2020

dashpole commented Mar 28, 2020

dashpole commented Oct 1, 2020

kikisdeliveryservice left a comment

dashpole commented Oct 8, 2020

lavalamp Oct 8, 2020

dashpole Oct 8, 2020

lavalamp Oct 8, 2020

lavalamp Oct 8, 2020

dashpole Oct 8, 2020

lavalamp Oct 8, 2020

lavalamp Oct 8, 2020

lavalamp Oct 8, 2020

dashpole Oct 8, 2020

lavalamp Oct 8, 2020

lavalamp Oct 8, 2020

dashpole Oct 8, 2020

evankanderson commented Oct 8, 2020

evankanderson commented Oct 8, 2020

dashpole commented Oct 8, 2020

lavalamp commented Oct 8, 2020

k8s-ci-robot commented Oct 8, 2020

dashpole commented Oct 8, 2020

lavalamp commented Oct 8, 2020

dashpole commented Oct 8, 2020


		### Test Plan

		We will e2e test this feature by deploying an OpenTelemetry Collector on the master, and configure it to export traces using the [stdout exporter](https://github.com/open-telemetry/opentelemetry-go/tree/master/exporters/stdout), which logs the spans in json format. We can then verify that the logs contain our expected traces when making calls to the API Server.


		We will wrap the API Server's http server and http clients with [otelhttp](https://github.com/open-telemetry/opentelemetry-go-contrib/tree/master/instrumentation/net/http/otelhttp) to get spans for incoming and outgoing http requests. This generates spans for all sampled incoming requests and propagates context with all client requests. For incoming requests, this would go below [WithRequestInfo](https://github.com/kubernetes/kubernetes/blob/9eb097c4b07ea59c674a69e19c1519f0d10f2fa8/staging/src/k8s.io/apiserver/pkg/server/config.go#L676) in the filter stack, as it must be after authentication and authorization, before the panic filter, and is closest in function to the WithRequestInfo filter.

		Note that some clients of the API Server, such as webhooks, may make reentrant calls to the API Server. To gain the full benefit of tracing, such clients should propagate context with requests back to the API Server.


		This KEP proposes the use of the [OpenTelemetry tracing framework](https://opentelemetry.io/) to create and export spans to configured backends.

		The API Server will use the [OpenTelemetry exporter format](https://github.com/open-telemetry/opentelemetry-proto), and the [OTlp exporter](https://github.com/open-telemetry/opentelemetry-go/tree/master/exporters/otlp#opentelemetry-collector-go-exporter) which can export traces. This format is easy to use with the [OpenTelemetry Collector](https://github.com/open-telemetry/opentelemetry-collector), which allows importing and configuring exporters for trace storage backends to be done out-of-tree in addition to other useful features.

Tracing KEP: Move to Implementable #1458

Tracing KEP: Move to Implementable #1458

Conversation

dashpole commented Jan 16, 2020

dashpole commented Mar 16, 2020

dashpole commented Mar 16, 2020

brancz commented Mar 24, 2020

dashpole commented Mar 24, 2020

serathius commented Mar 24, 2020

dashpole commented Mar 28, 2020

dashpole commented Oct 1, 2020

kikisdeliveryservice left a comment

Choose a reason for hiding this comment

dashpole commented Oct 8, 2020

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

evankanderson commented Oct 8, 2020

evankanderson commented Oct 8, 2020

dashpole commented Oct 8, 2020

lavalamp commented Oct 8, 2020

k8s-ci-robot commented Oct 8, 2020

dashpole commented Oct 8, 2020

lavalamp commented Oct 8, 2020

dashpole commented Oct 8, 2020