[mixer] Introduce tracespan template into Mixer #1349

douglas-reid · 2017-11-03T19:06:16Z

What this PR does / why we need it:

This PR adds a template for trace spans to Mixer. A trace span template enables the integration of tracing adapters into Mixer. Early prototyping using this template demonstrated success in deriving tracing spans from proxy Report()s for services for jaeger, zipkin, and stackdriver backends. Operators can control the level of detail forwarded to tracing backends (as well as span names, for instance) using this template. The template also lays the groundwork for supporting Mixer ingestion of trace span data.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

fixes istio/old_mixer_repo#797, fixes: istio/old_mixer_repo#1509

Special notes for your reviewer:

Release note:

Tracing template added to Mixer

guptasu

Took a quick pass. Looks good.

guptasu · 2017-11-03T19:48:41Z

mixer/template/tracespan/tracespan.proto

+//   name: default
+//   namespace: istio-system
+// spec:
+//   traceId: request.headers["x-b3-traceid"]


These should be snake case right ? May be camelCase might work too, what is our recommendation ?

I was under the impression we wanted "human case" config, where camelCase was preferred to snake_case. But I actually am not certain. @geeknoid ?

I think camelCase is what we're using.

Thanks for clarifying.

guptasu · 2017-11-03T19:53:13Z

mixer/template/tracespan/tracespan.proto

+    // being described by this message. If this is a root span, then this field
+    // MUST be empty.
+    //
+    // Optional.


This cannot be auto generated. Consider explaining what will happen if this is not provided ?

guptasu · 2017-11-03T19:53:55Z

mixer/template/tracespan/tracespan.proto

+    string span_id = 2;
+
+    // Parent Span ID is the unique identifier for a parent span of the span
+    // being described by this message. If this is a root span, then this field


by this span instance ?

guptasu · 2017-11-03T19:55:11Z

mixer/template/tracespan/tracespan.proto

+
+    // The start time of the span.
+    //
+    // Required: This field MUST be configured in generated instances.


consider removing "in generated instances". May be remove the entire sentence.. just Required is sufficient, unless we want to explain why is this required.

guptasu · 2017-11-03T19:57:39Z

mixer/template/tracespan/tracespan.proto

+    // Examples:
+    //   "http.method" : "POST"
+    //   "peer.service" : "foo.default.svc.cluster.local"
+    //   "request_size_bytes" : 234


should we make this request.size ? and "peer.service" : source.service ?

Or just copy the same example from above

I've removed the examples altogether, as they overlap with example attribute expressions.

guptasu · 2017-11-03T20:01:17Z

mixer/template/tracespan/tracespan.proto

+//   namespace: istio-system
+// spec:
+//   traceId: request.headers["x-b3-traceid"]
+//   spanId: request.headers["x-b3-spanid"] | ""


are the x-b3-spanid and x-b3-parentspanid well known header names. If we expect operators to use these for most case, consider explaining a bit about whey those tags mean.

these are well-known header names (from the zipkin set of headers). they are described in full on our distributed tracing task. I've added a link to that Task here.

Cool, that link will be helpful.

guptasu · 2017-11-03T20:03:48Z

mixer/template/tracespan/tracespan.proto

+
+option (istio.mixer.v1.template.template_variety) = TEMPLATE_VARIETY_REPORT;
+
+// TraceSpan represents an individual span within a distributed trace.


Should this be Trace span instead of TraceSpan?

Are you suggesting renaming the package, or just wondering if I should put a space in between the two words? This naming matches LogEntry.

I see, I was referring to the absence of space between the two words. Let's stick to LogEntry format then.

geeknoid

Looks good.

douglas-reid

PTAL.

douglas-reid · 2017-11-06T18:35:09Z

mixer/template/tracespan/tracespan.proto

+
+option (istio.mixer.v1.template.template_variety) = TEMPLATE_VARIETY_REPORT;
+
+// TraceSpan represents an individual span within a distributed trace.


Are you suggesting renaming the package, or just wondering if I should put a space in between the two words? This naming matches LogEntry.

douglas-reid · 2017-11-06T18:36:13Z

mixer/template/tracespan/tracespan.proto

+//   name: default
+//   namespace: istio-system
+// spec:
+//   traceId: request.headers["x-b3-traceid"]


I was under the impression we wanted "human case" config, where camelCase was preferred to snake_case. But I actually am not certain. @geeknoid ?

douglas-reid · 2017-11-06T18:38:06Z

mixer/template/tracespan/tracespan.proto

+//   namespace: istio-system
+// spec:
+//   traceId: request.headers["x-b3-traceid"]
+//   spanId: request.headers["x-b3-spanid"] | ""


these are well-known header names (from the zipkin set of headers). they are described in full on our distributed tracing task. I've added a link to that Task here.

douglas-reid · 2017-11-06T18:40:49Z

mixer/template/tracespan/tracespan.proto

+    // Examples:
+    //   "http.method" : "POST"
+    //   "peer.service" : "foo.default.svc.cluster.local"
+    //   "request_size_bytes" : 234


I've removed the examples altogether, as they overlap with example attribute expressions.

douglas-reid · 2017-11-06T18:47:38Z

mixer/template/tracespan/tracespan.proto

+
+    // The start time of the span.
+    //
+    // Required: This field MUST be configured in generated instances.


guptasu

LGTM.

guptasu · 2017-11-06T19:48:26Z

mixer/template/tracespan/tracespan.proto

+
+option (istio.mixer.v1.template.template_variety) = TEMPLATE_VARIETY_REPORT;
+
+// TraceSpan represents an individual span within a distributed trace.


I see, I was referring to the absence of space between the two words. Let's stick to LogEntry format then.

guptasu · 2017-11-06T19:49:11Z

mixer/template/tracespan/tracespan.proto

+//   name: default
+//   namespace: istio-system
+// spec:
+//   traceId: request.headers["x-b3-traceid"]


Thanks for clarifying.

guptasu · 2017-11-06T19:49:53Z

mixer/template/tracespan/tracespan.proto

+//   namespace: istio-system
+// spec:
+//   traceId: request.headers["x-b3-traceid"]
+//   spanId: request.headers["x-b3-spanid"] | ""


Cool, that link will be helpful.

guptasu · 2017-11-06T19:54:55Z

mixer/template/tracespan/tracespan.proto

+    // Required.
+    string trace_id = 1;
+
+    // Span ID is the unique identifier for a span within a trace. It is assigned


Is this auto assigned if not provided ?

Yes. Although, I think that is an adapter detail. Certainly, proper implementations will auto-assign.

guptasu · 2017-11-06T19:55:53Z

/lgtm

istio-merge-robot · 2017-11-07T23:27:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guptasu

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

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

~~mixer/OWNERS~~ [guptasu]

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

Automatic merge from submit-queue remove nodeport from eureka service **What this PR does / why we need it**: Removes `NodePort` designation from Eureka service. This might cause issues with the automated tests if it is not removed. Fortunately, no Eureka tests have been enabled yet, so it shouldn't be causing issues right now. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # **Special notes for your reviewer**: **Release note**: ```release-note NONE ```

* Introduce tracespan template into Mixer * Address review comments * Add generated files

istio-testing added the release-note label Nov 3, 2017

googlebot added the cla: yes label Nov 3, 2017

douglas-reid assigned mandarjog, guptasu and geeknoid Nov 3, 2017

guptasu reviewed Nov 3, 2017

View reviewed changes

geeknoid reviewed Nov 3, 2017

View reviewed changes

douglas-reid commented Nov 6, 2017

View reviewed changes

guptasu approved these changes Nov 6, 2017

View reviewed changes

istio-testing added the lgtm label Nov 6, 2017

istio-merge-robot added the approved label Nov 7, 2017

rshriram changed the title ~~Introduce tracespan template into Mixer~~ [mixer] Introduce tracespan template into Mixer Nov 8, 2017

ZackButcher added the mixer label Nov 8, 2017

douglas-reid added 3 commits November 9, 2017 09:50

Introduce tracespan template into Mixer

8682a0b

Address review comments

30494dc

Add generated files

f6a7abe

douglas-reid force-pushed the trace-template branch from e44ce9b to f6a7abe Compare November 9, 2017 18:05

istio-merge-robot removed the approved label Nov 9, 2017

douglas-reid merged commit 9377ba0 into istio:master Nov 9, 2017

vadimeisenbergibm pushed a commit to vadimeisenbergibm/istio that referenced this pull request Nov 10, 2017

[mixer] Introduce tracespan template into Mixer (istio#1349)

962c115

* Introduce tracespan template into Mixer * Address review comments * Add generated files

jamsajones pushed a commit to jamsajones/istio that referenced this pull request Jan 3, 2018

[mixer] Introduce tracespan template into Mixer (istio#1349)

df6649a

* Introduce tracespan template into Mixer * Address review comments * Add generated files

douglas-reid deleted the trace-template branch June 18, 2018 17:20

0x01001011 pushed a commit to thedemodrive/istio that referenced this pull request Jul 16, 2020

Generate with latest build tool (istio#1349)

11f0c72

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mixer] Introduce tracespan template into Mixer #1349

[mixer] Introduce tracespan template into Mixer #1349

douglas-reid commented Nov 3, 2017 •

edited

Loading

guptasu left a comment

guptasu Nov 3, 2017

douglas-reid Nov 6, 2017

geeknoid Nov 6, 2017

guptasu Nov 6, 2017

guptasu Nov 3, 2017

guptasu Nov 3, 2017

guptasu Nov 3, 2017

douglas-reid Nov 6, 2017

guptasu Nov 3, 2017

guptasu Nov 3, 2017

douglas-reid Nov 6, 2017

guptasu Nov 3, 2017

douglas-reid Nov 6, 2017

guptasu Nov 6, 2017

guptasu Nov 3, 2017

douglas-reid Nov 6, 2017

guptasu Nov 6, 2017

geeknoid left a comment

douglas-reid left a comment

douglas-reid Nov 6, 2017

douglas-reid Nov 6, 2017

douglas-reid Nov 6, 2017

douglas-reid Nov 6, 2017

douglas-reid Nov 6, 2017

guptasu left a comment

guptasu Nov 6, 2017

guptasu Nov 6, 2017

guptasu Nov 6, 2017

guptasu Nov 6, 2017

douglas-reid Nov 6, 2017

guptasu commented Nov 6, 2017

istio-merge-robot commented Nov 7, 2017


		option (istio.mixer.v1.template.template_variety) = TEMPLATE_VARIETY_REPORT;

		// TraceSpan represents an individual span within a distributed trace.

[mixer] Introduce tracespan template into Mixer #1349

[mixer] Introduce tracespan template into Mixer #1349

Conversation

douglas-reid commented Nov 3, 2017 • edited Loading

guptasu 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geeknoid left a comment

Choose a reason for hiding this comment

douglas-reid 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

guptasu 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

guptasu commented Nov 6, 2017

istio-merge-robot commented Nov 7, 2017

douglas-reid commented Nov 3, 2017 •

edited

Loading