tracing: Update docs to clarify situation with trace context propagation by objectiser · Pull Request #1733 · envoyproxy/envoy

objectiser · 2017-09-25T17:49:29Z

resolves #1711

@mattklein123 @adriancole @bhs @louiscryan

mattklein123

Thanks @objectiser this looks great to me. Will defer to @adriancole on whether the text is OK.

mattklein123 · 2017-09-25T18:23:24Z

docs/intro/arch_overview/tracing.rst

nit: reflow to ~100 col.

rshriram

Minor nits on my end. Thanks for doing this!
LGTM

rshriram · 2017-09-25T19:23:19Z

docs/intro/arch_overview/tracing.rst

Nit: inbound and outbound instead of ingress vs egress. But if you feel that this correlates better with the ingress and egress operations, then leave it be.

rshriram · 2017-09-25T19:25:36Z

docs/intro/arch_overview/tracing.rst

Services can emit additional traces over the standard traces emitted by Envoy. However, it is the responsibility of the service to transport these traces to the trace collector...

rshriram · 2017-09-25T19:26:31Z

docs/intro/arch_overview/tracing.rst

Oh you have described it here.. may be nice this part to the top? To start the paragraph with the crux of this discussion

objectiser · 2017-09-25T20:27:38Z

@rshriram I've just applied the minor changes for now - if others also think the use of tracer in the service should be moved above the manual propagation, then I've no problem switching it around.

mattklein123 · 2017-09-25T22:44:33Z

I don't have a strong opinion on additional moves. @objectiser can you merge master so the Circle tests get run?

objectiser · 2017-09-26T08:10:49Z

@rshriram I've reorganized the section to put the discussion on use of tracer in the service first, followed by manual propagation - but on re-reading your comment I think I misinterpreted your suggestion. Could you have a quick review of this change and let me know your thoughts?

The reason I originally put the emphasis on use of tracer in service for context propagation is that was the focus of this section of the doc - but the additional benefit is that it also enables the service to provide additional tracing information.

rshriram

This reads very well!

mattklein123

Very nice. Thank you for making the docs better.

bhs

@objectiser two nits, neither of which I feel strongly about!

bhs · 2017-09-26T17:52:25Z

docs/configuration/http_conn_man/headers.rst

pedantic nitpick:

`binary "basictracer" < ... >` carrier

(OpenTracing per se does not have an "official" encoding here)

This text was in the original version - I was actually thinking about removing it as the service developer does not need to know anything about the format.

I'll remove and if anyone has any objections, I can add some modified text back.

bhs · 2017-09-26T17:57:51Z

docs/intro/arch_overview/tracing.rst

nit: ... outbound requests. (Note: for LightStep, the above is accomplished via OpenTracing; there should be no need to invoke LightStep's tracer directly for extraction or injection of context state) This approach would ...

(Or any other edit that makes it clear that LightStep's tracer is abstracted underneath OpenTracing's API)

I've added LightStep (via OpenTracing API) - let me know if that is ok.

mattklein123 · 2017-09-27T16:41:40Z

@objectiser thanks LGTM. Can you merge master to fix release CI issue and also fix DCO?

objectiser · 2017-09-27T19:44:25Z

@mattklein123 Just noticed the remaining circleci jobs have been cancelled - is there a problem?

mattklein123 · 2017-09-27T19:49:15Z

Sorry please merge master again. Fixing CI issues.

resolves envoyproxy#1711 Signed-off-by: Gary Brown <gary@brownuk.com>

…ion (envoyproxy#1733) resolves envoyproxy#1711 Signed-off-by: Gary Brown <gary@brownuk.com>

* Update_Dependencies (envoyproxy#1657) * Use mTLS mode enum to decide authentication result. * Clang * Use NOT_REACHED macros for default case. * Clang

Description: This adds a dumpStats function to the Engine interfaces, allowing callers to retrieve the value of all active stats. This is done by invoking an admin handler, and so this PR also sets up a pattern for synchronously calling the admin handler. This should make it easier to add support for other features that would be based on invoking admin endpoints. Risk Level: Medium Testing: New integration tests Docs Changes: n/a Release Notes: n/a Signed-off-by: Snow Pettersen <snowp@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Description** All directory under tests/ have exactly one corresponding Make recipe except for this one, so this moves it under the correct tests/data-plane-mcp. Plus the directory name was too broad as it's only for MCP, not AI data plane path. --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

objectiser force-pushed the tracedoc branch from 0e18032 to a421690 Compare September 25, 2017 17:50

mattklein123 reviewed Sep 25, 2017

View reviewed changes

docs/intro/arch_overview/tracing.rst Outdated

Copy link
Copy Markdown

Member

mattklein123 Sep 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: reflow to ~100 col.

rshriram approved these changes Sep 25, 2017

View reviewed changes

objectiser force-pushed the tracedoc branch from 1e429a8 to f52c823 Compare September 25, 2017 20:28

objectiser force-pushed the tracedoc branch from 9415510 to 001f191 Compare September 26, 2017 08:01

rshriram reviewed Sep 26, 2017

View reviewed changes

mattklein123 previously approved these changes Sep 26, 2017

View reviewed changes

bhs approved these changes Sep 26, 2017

View reviewed changes

objectiser dismissed mattklein123’s stale review via 67e2a82 September 27, 2017 08:43

objectiser force-pushed the tracedoc branch from 236c4ff to 82a1e29 Compare September 27, 2017 16:57

mattklein123 previously approved these changes Sep 27, 2017

View reviewed changes

tracing: Update docs to clarify situation with trace context propagation

c22c6d5

resolves envoyproxy#1711 Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser dismissed mattklein123’s stale review via c22c6d5 September 27, 2017 20:24

objectiser force-pushed the tracedoc branch from 82a1e29 to c22c6d5 Compare September 27, 2017 20:24

mattklein123 approved these changes Sep 27, 2017

View reviewed changes

mattklein123 merged commit c52afae into envoyproxy:master Sep 27, 2017

costinm pushed a commit to costinm/envoy that referenced this pull request Oct 2, 2017

tracing: Update docs to clarify situation with trace context propagat…

dfd99ad

…ion (envoyproxy#1733) resolves envoyproxy#1711 Signed-off-by: Gary Brown <gary@brownuk.com>

Conversation

objectiser commented Sep 25, 2017

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

objectiser commented Sep 25, 2017

Uh oh!

mattklein123 commented Sep 25, 2017

Uh oh!

objectiser commented Sep 26, 2017

Uh oh!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

bhs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Sep 27, 2017

Uh oh!

objectiser commented Sep 27, 2017

Uh oh!

mattklein123 commented Sep 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants