tracing: add baggage methods to Tracing::Span by matthewgrossman · Pull Request #12260 · envoyproxy/envoy

matthewgrossman · 2020-07-23T20:03:03Z

Commit Message: tracing: add baggage methods to Tracing::Span
Additional Description: references #11622: We want an envoy filter to be able to read information off of the OT baggage to guide some routing decisions. First step is exposing the functionality of the existing spans/tracers so we can access this baggage dict within the filters. Once this functionality is added to the Tracing::Span, we can access with the baggage info inside a filter
Risk Level: Low: Small optional feature
Testing: Unit testing is added for all derived classes of Tracing::Span. All of the classes are noops except OpenTracingSpan (test) and LightStep (test)
Docs Changes: N/A for now, should something about baggage be added to tracing.rst?
Release Notes: think we need a note here?

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

…th-baggage Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

source/extensions/tracers/common/ot/opentracing_driver_impl.cc

kyessenov · 2020-07-23T20:08:22Z

include/envoy/tracing/http_tracer.h

+   * @param key baggage key
+   * @param key baggage value
+   */
+  virtual void setBaggage(const std::string& key, const std::string& value) PURE;


What's the difference with setTag?

baggage is persisted in the SpanContext, which means that any subsequent spans can access data in the baggage (it gets injected/extracted across process boundaries). This is useful when you need to embed information that is available for the entirety of the request

more reading: https://opentracing.io/docs/overview/tags-logs-baggage/, https://github.com/opentracing/specification/blob/master/specification.md#set-a-baggage-item

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

source/extensions/tracers/xray/tracer.h

LisaLudique

looks good so far, pending unit tests for the tracers and adding/updating documentation (it should be clear where baggage is/isn't supported)

include/envoy/tracing/http_tracer.h

LisaLudique · 2020-07-24T18:05:27Z

source/extensions/tracers/opencensus/opencensus_tracer_impl.cc

+void Span::setBaggage(const std::string&, const std::string&) {}
+
+std::string Span::getBaggage(const std::string&) { return std::string(); }


consider logging a warning here that this tracer doesn't support baggage (and likewise for the other tracers that do not support it)

you could even log an error saying baggage isn't supported similar to

envoy/source/extensions/tracers/opencensus/opencensus_tracer_impl.cc

Line 358 in 68df9d1

ENVOY_LOG(error, "RateLimitingSampler is not supported.");

A warning isn't safe in the data path. I would either do nothing and leave a TODO or have a stat for dropped baggage. Same elsewhere.

I left comments for the spans that don't support baggage (opencensus, xray) and a TODO for zipkin

mattklein123

Thanks for working on this! I will defer to @LisaLudique for initial review.

/wait

mattklein123 · 2020-07-24T18:15:49Z

source/extensions/tracers/opencensus/opencensus_tracer_impl.cc

+void Span::setBaggage(const std::string&, const std::string&) {}
+
+std::string Span::getBaggage(const std::string&) { return std::string(); }


A warning isn't safe in the data path. I would either do nothing and leave a TODO or have a stat for dropped baggage. Same elsewhere.

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

matthewgrossman · 2020-07-30T02:50:17Z

tuned up the PR with tests/comments/todos, tried to get the description to match the template as well. Might be worth reading that again since it's likely changed since last review.

Do we need to express anywhere else that not all implementations of Tracing::Span currently support the baggage methods? Maybe that's what would be valuable somewhere in some documentation

LisaLudique · 2020-07-31T21:39:19Z

tests look good!

I would suggest adding a section on baggage and delineating the level of support for each tracer in https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/observability/tracing

source/extensions/tracers/zipkin/zipkin_tracer_impl.cc

test/extensions/tracers/xray/tracer_test.cc

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

mattklein123

LGTM with small comment. Thank you!

/wait

mattklein123 · 2020-08-10T20:00:31Z

source/extensions/tracers/opencensus/opencensus_tracer_impl.cc

+void Span::setBaggage(absl::string_view, absl::string_view) {}
+
+std::string Span::getBaggage(absl::string_view) { return std::string(); }


nit: just inline this above.

Signed-off-by: Jake Kaufman <jkaufman@lyft.com>

mattklein123

Thanks!

matthewgrossman · 2020-08-11T00:50:39Z

thanks for the help in review!

matthewgrossman added 6 commits July 21, 2020 16:06

WIP add some header info on baggage

0fc9517

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

messing w/ stuff

d22aa9e

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

get a compiling solution

2d2a59b

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

Merge remote-tracking branch 'upstream/master' into mg-play-around-wi…

e8db6dd

…th-baggage Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

lint

78a6870

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

format again

41d1323

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

matthewgrossman commented Jul 23, 2020

View reviewed changes

source/extensions/tracers/common/ot/opentracing_driver_impl.cc Outdated Show resolved Hide resolved

kyessenov reviewed Jul 23, 2020

View reviewed changes

mattklein123 self-assigned this Jul 23, 2020

matthewgrossman added 2 commits July 23, 2020 16:14

formatting

9259977

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

change the test slightly

68df9d1

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

matthewgrossman commented Jul 24, 2020

View reviewed changes

source/extensions/tracers/xray/tracer.h Outdated Show resolved Hide resolved

LisaLudique reviewed Jul 24, 2020

View reviewed changes

mattklein123 requested changes Jul 24, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 24, 2020

mattklein123 assigned LisaLudique Jul 24, 2020

s/std::string/string_view

92e862f

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

repokitteh-read-only bot removed the waiting label Jul 24, 2020

add note about baggage and child/parent spans

4abed0b

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

mattklein123 added the waiting label Jul 27, 2020

add in test for OpenTracingDriverTest

dd0933a

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

repokitteh-read-only bot removed the waiting label Jul 29, 2020

matthewgrossman added 3 commits July 29, 2020 11:01

add in test for OpenCensus

e33c479

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

add in baggage test for xray

fe78194

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

add in zipkin tests

6004faa

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

mattklein123 added the waiting:any label Jul 30, 2020

add in http tracer impl test

bad6068

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

repokitteh-read-only bot removed the waiting:any label Jul 30, 2020

fix test for nulltracer

e75f095

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

matthewgrossman changed the title ~~WIP Add opentracing baggage methods~~ tracing: add baggage methods to Tracing::Span Jul 30, 2020

matthewgrossman marked this pull request as ready for review July 30, 2020 02:46

LisaLudique reviewed Jul 31, 2020

View reviewed changes

source/extensions/tracers/zipkin/zipkin_tracer_impl.cc Show resolved Hide resolved

LisaLudique reviewed Jul 31, 2020

View reviewed changes

test/extensions/tracers/xray/tracer_test.cc Show resolved Hide resolved

mattklein123 added the waiting label Jul 31, 2020

Add additional TODO for zipkin impl and add in comments for tests

05dbaa5

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

repokitteh-read-only bot removed the waiting label Aug 5, 2020

matthewgrossman added 2 commits August 4, 2020 22:46

add in draft docs

446ef0a

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

fix doc link

f86f0f8

Signed-off-by: Matthew Grossman <matthewryangrossman@gmail.com>

LisaLudique previously approved these changes Aug 10, 2020

View reviewed changes

mattklein123 requested changes Aug 10, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 10, 2020

theevocater dismissed LisaLudique’s stale review via fadce22 August 10, 2020 21:53

repokitteh-read-only bot removed the waiting label Aug 10, 2020

theevocater force-pushed the mg-play-around-with-baggage branch from fadce22 to adcf01e Compare August 10, 2020 21:55

Inline empty methods per mklein's nit

a277e37

Signed-off-by: Jake Kaufman <jkaufman@lyft.com>

theevocater force-pushed the mg-play-around-with-baggage branch from adcf01e to a277e37 Compare August 10, 2020 23:11

mattklein123 approved these changes Aug 11, 2020

View reviewed changes

mattklein123 merged commit 9ad7d4c into envoyproxy:master Aug 11, 2020

theevocater deleted the mg-play-around-with-baggage branch August 11, 2020 14:50

CodeBlanch mentioned this pull request Aug 11, 2020

Baggage + CorrelationContext improvements by Eddy & Mike open-telemetry/opentelemetry-dotnet#1048

Merged

3 tasks

matthewgrossman mentioned this pull request Aug 18, 2020

tracing: support getting/setting baggage on spans #11622

Closed

		void Span::setBaggage(const std::string&, const std::string&) {}

		std::string Span::getBaggage(const std::string&) { return std::string(); }

		void Span::setBaggage(absl::string_view, absl::string_view) {}

		std::string Span::getBaggage(absl::string_view) { return std::string(); }

Conversation

matthewgrossman commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LisaLudique left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewgrossman commented Jul 30, 2020

Uh oh!

LisaLudique commented Jul 31, 2020

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

matthewgrossman commented Aug 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

matthewgrossman commented Jul 23, 2020 •

edited

Loading