add open telemetry #2530

MarcelHillmann · 2020-05-01T14:00:27Z

Hi,

the second try for #2516 without Zipkin ;)

Thanks
Marcel

k8s-ci-robot · 2020-05-01T14:00:36Z

Hi @MarcelHillmann. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

dashpole · 2020-05-01T18:25:35Z

/ok-to-test

dashpole · 2020-05-01T22:12:20Z

cmd/internal/opentelemetry/wrapperMux.go

+)
+
+func init() {
+	flag.Var(&traceTags, "trace_tags", "additional tags for tracing (eg. foo=bar)")


What is the use case for adding tags via flags?

If you need more flags for the search to differ the Traces.

Like k8s.* oder cloud.* https://github.com/open-telemetry/opentelemetry-specification/tree/master/specification/resource/semantic_conventions

I would prefer discovering these from the host wherever possible, rather than setting them via flags. If we do go the route of flags, i'd prefer specific flags for each attribute, rather than a k/v list. But i'm hoping we don't have to do that.

cmd/internal/opentelemetry/wrapperMux.go

googlebot · 2020-05-02T10:18:41Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

MarcelHillmann · 2020-05-02T10:39:25Z

@googlebot I fixed it.

MarcelHillmann · 2020-05-02T10:51:54Z

@googlebot I fixed it.

googlebot · 2020-05-02T14:25:17Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

fix typo padding left add logging generic Extractor simplify Signed-off-by: Marcel <[email protected]>

Signed-off-by: Marcel <[email protected]>

MarcelHillmann · 2020-05-14T19:05:31Z

/retest

MarcelHillmann · 2020-05-15T15:31:44Z

/retest

Signed-off-by: Marcel <[email protected]>

iwankgb · 2020-05-18T20:35:55Z

cmd/internal/opentelemetry/wrapperMux.go

+package opentelemetry
+
+import (
+	"context"


Could you run goimports on this file, please?

iwankgb · 2020-05-18T20:37:02Z

cmd/internal/opentelemetry/wrapperMux.go

+		klog.Fatal(err)
+	}
+
+	sampler := getTraceSamplerProbability() //*traceSampler, *traceSamplerParam)


Remove the comment, please.

iwankgb · 2020-05-18T20:40:50Z

cmd/internal/opentelemetry/wrapperMux.go

+func getTraceSamplerProbability() trace.Sampler {
+	probability := *traceProbability
+	if probability < 0 && probability > 1 {
+		klog.Errorf("invalid probability value %f, probability is allowed between 0.0 and 1.0", probability)


Shouldn't you return trace.NeverSample() here? Otherwise we are going to hit return trace.ProbabilitySampler(probability) with invalid probability.

iwankgb · 2020-05-18T20:44:45Z

cmd/cadvisor.go

-	mux := http.NewServeMux()
+	// setup open telemetry
+	if *traceEndpoint != "" {
+		defer opentelemetry.InitTrace(*traceEndpoint, *collectorCert).Stop()


I think I understand why Stop() call is deferred but why is InitTrace() call deferred?

iwankgb · 2020-05-18T20:48:12Z

cmd/internal/opentelemetry/wrapperMux.go

+	}
+	global.SetTraceProvider(traceProvider)
+
+	apiB3Single, apiB3, traceCtx, corrCtx := apiTrace.B3{SingleHeader: false}, apiTrace.B3{SingleHeader: false}, apiTrace.DefaultHTTPPropagator(), correlation.DefaultHTTPPropagator()


How is apiB3Single different from apiB3? They seem to be identically configured instance of the same struct.

iwankgb · 2020-05-18T20:50:42Z

cmd/internal/opentelemetry/wrapperMux.go

+	}
+}
+
+func getTraceTags(tags []string) []kv.KeyValue {


This function seems to be unused.

iwankgb · 2020-05-18T20:52:20Z

cmd/internal/opentelemetry/wrapperMux.go

+	return keyValue
+}
+
+func ToString(keyValues []kv.KeyValue) string {


What is this function needed for? Are you trying to generate some Golang code?

dashpole · 2020-05-28T17:37:53Z

cc @SergeyKanzhelev

SergeyKanzhelev

It looks like a first step of adding opentelemetry. I left some comments. Looking forward to see how this will be used

SergeyKanzhelev · 2020-05-29T04:20:31Z

cmd/internal/opentelemetry/tracingTags.go

+	"strings"
+)
+
+type tracingTags []string


I cannot find where it's used

SergeyKanzhelev · 2020-05-29T04:21:02Z

cmd/go.mod

@@ -23,18 +23,19 @@ require (
 	github.com/golang/snappy v0.0.0-20150730031844-723cc1e459b8 // indirect
 	github.com/influxdb/influxdb v0.9.6-0.20151125225445-9eab56311373
 	github.com/klauspost/crc32 v0.0.0-20151223135126-a3b15ae34567 // indirect
-	github.com/kr/pretty v0.1.0 // indirect


curious why adding opentelemetry removed this indirect dependency

SergeyKanzhelev · 2020-05-29T04:26:06Z

cmd/internal/opentelemetry/wrapperMux.go

+
+const traceServiceName = "cadvisor"
+
+var traceProbability = flag.Float64("trace_probability", 1, "used for sampler (0=never, 1=always and probability is 0.1 to 0.99)")


I see trace_ prefix is used here and for the OTLP collector address. If variables would be standardized in OpenTelemetry, would you be open to reuse names from there instead of inventing new names? Or these flags has it's own naming convention?

SergeyKanzhelev · 2020-05-29T04:37:43Z

cmd/internal/opentelemetry/wrapperMux.go

+func getTraceTags(tags []string) []kv.KeyValue {
+	keyValue := []kv.KeyValue{
+		kv.String("service.name", traceServiceName),
+		kv.String("service.version", fmt.Sprintf("semver: %s git:%s", version.Info["version"], version.Info["revision"])),


I think the idea is either use one or another, not two together. Do you need both?

SergeyKanzhelev · 2020-05-29T04:46:12Z

cmd/cadvisor.go

@@ -77,6 +78,8 @@ var rawCgroupPrefixWhiteList = flag.String("raw_cgroup_prefix_whitelist", "", "A

 var perfEvents = flag.String("perf_events_config", "", "Path to a JSON file containing configuration of perf events to measure. Empty value disabled perf events measuring.")

+var traceEndpoint = flag.String("trace_endpoint", "", "Url host:port to the OTLP collector")


why are you reading this variable here? Would it be better to hide this logic inside the InitTrace?

bobbypage · 2021-01-22T01:57:49Z

I'm going to close this for now since there hasn't been any updates in a while, please re-open if appropriate.

k8s-ci-robot added the needs-ok-to-test label May 1, 2020

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels May 1, 2020

dashpole reviewed May 1, 2020

View reviewed changes

MarcelHillmann force-pushed the feature/otlp branch from 6ad8b9f to c54c2de Compare May 2, 2020 14:25

add open telemetry

a826391

fix typo padding left add logging generic Extractor simplify Signed-off-by: Marcel <[email protected]>

MarcelHillmann force-pushed the feature/otlp branch from c54c2de to a826391 Compare May 3, 2020 07:26

fix review

9536414

Signed-off-by: Marcel <[email protected]>

fix brocken code

6e34411

Signed-off-by: Marcel <[email protected]>

iwankgb reviewed May 18, 2020

View reviewed changes

SergeyKanzhelev reviewed May 29, 2020

View reviewed changes

dashpole self-assigned this Aug 13, 2020

bobbypage closed this Jan 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add open telemetry #2530

add open telemetry #2530

MarcelHillmann commented May 1, 2020

k8s-ci-robot commented May 1, 2020

dashpole commented May 1, 2020

dashpole May 1, 2020

MarcelHillmann May 2, 2020

dashpole May 8, 2020

googlebot commented May 2, 2020

MarcelHillmann commented May 2, 2020

MarcelHillmann commented May 2, 2020

googlebot commented May 2, 2020

MarcelHillmann commented May 14, 2020

MarcelHillmann commented May 15, 2020

iwankgb May 18, 2020

iwankgb May 18, 2020

iwankgb May 18, 2020

iwankgb May 18, 2020

iwankgb May 18, 2020

iwankgb May 18, 2020

iwankgb May 18, 2020

dashpole commented May 28, 2020

SergeyKanzhelev left a comment

SergeyKanzhelev May 29, 2020

SergeyKanzhelev May 29, 2020

SergeyKanzhelev May 29, 2020

SergeyKanzhelev May 29, 2020

SergeyKanzhelev May 29, 2020

bobbypage commented Jan 22, 2021


		const traceServiceName = "cadvisor"

		var traceProbability = flag.Float64("trace_probability", 1, "used for sampler (0=never, 1=always and probability is 0.1 to 0.99)")

		@@ -77,6 +78,8 @@ var rawCgroupPrefixWhiteList = flag.String("raw_cgroup_prefix_whitelist", "", "A

		var perfEvents = flag.String("perf_events_config", "", "Path to a JSON file containing configuration of perf events to measure. Empty value disabled perf events measuring.")

		var traceEndpoint = flag.String("trace_endpoint", "", "Url host:port to the OTLP collector")

add open telemetry #2530

add open telemetry #2530

Conversation

MarcelHillmann commented May 1, 2020

k8s-ci-robot commented May 1, 2020

dashpole commented May 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented May 2, 2020

MarcelHillmann commented May 2, 2020

MarcelHillmann commented May 2, 2020

googlebot commented May 2, 2020

MarcelHillmann commented May 14, 2020

MarcelHillmann commented May 15, 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

dashpole commented May 28, 2020

SergeyKanzhelev 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

bobbypage commented Jan 22, 2021