Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tracing: add baggage methods to Tracing::Span #12260

Merged
merged 20 commits into from
Aug 11, 2020
Merged

tracing: add baggage methods to Tracing::Span #12260

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
0fc9517
WIP add some header info on baggage
matthewgrossman Jul 21, 2020
d22aa9e
messing w/ stuff
matthewgrossman Jul 22, 2020
2d2a59b
get a compiling solution
matthewgrossman Jul 23, 2020
e8db6dd
Merge remote-tracking branch 'upstream/master' into mg-play-around-wi…
matthewgrossman Jul 23, 2020
78a6870
lint
matthewgrossman Jul 23, 2020
41d1323
format again
matthewgrossman Jul 23, 2020
9259977
formatting
matthewgrossman Jul 23, 2020
68df9d1
change the test slightly
matthewgrossman Jul 24, 2020
92e862f
s/std::string/string_view
matthewgrossman Jul 24, 2020
4abed0b
add note about baggage and child/parent spans
matthewgrossman Jul 24, 2020
dd0933a
add in test for OpenTracingDriverTest
matthewgrossman Jul 29, 2020
e33c479
add in test for OpenCensus
matthewgrossman Jul 29, 2020
fe78194
add in baggage test for xray
matthewgrossman Jul 29, 2020
6004faa
add in zipkin tests
matthewgrossman Jul 29, 2020
bad6068
add in http tracer impl test
matthewgrossman Jul 30, 2020
e75f095
fix test for nulltracer
matthewgrossman Jul 30, 2020
05dbaa5
Add additional TODO for zipkin impl and add in comments for tests
matthewgrossman Aug 5, 2020
446ef0a
add in draft docs
matthewgrossman Aug 5, 2020
f86f0f8
fix doc link
matthewgrossman Aug 7, 2020
a277e37
Inline empty methods per mklein's nit
theevocater Aug 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions include/envoy/tracing/http_tracer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,20 @@ class Span {
* @param sampled whether the span and any subsequent child spans should be sampled
*/
virtual void setSampled(bool sampled) PURE;

/**
* Retrieve a key's value from the current span's baggage
* @param key baggage key
* @return the baggage's value for the given input key
*/
virtual std::string getBaggage(const std::string& key) PURE;
matthewgrossman marked this conversation as resolved.
Show resolved Hide resolved

/**
* Set a key/value pair in the current span's baggage
matthewgrossman marked this conversation as resolved.
Show resolved Hide resolved
* @param key baggage key
* @param key baggage value
*/
virtual void setBaggage(const std::string& key, const std::string& value) PURE;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference with setTag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

};

/**
Expand Down
2 changes: 2 additions & 0 deletions source/common/tracing/http_tracer_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,8 @@ class NullSpan : public Span {
void log(SystemTime, const std::string&) override {}
void finishSpan() override {}
void injectContext(Http::RequestHeaderMap&) override {}
void setBaggage(const std::string&, const std::string&) override {}
std::string getBaggage(const std::string&) override { return std::string(); }
SpanPtr spawnChild(const Config&, const std::string&, SystemTime) override {
return SpanPtr{new NullSpan()};
}
Expand Down
6 changes: 6 additions & 0 deletions source/extensions/tracers/common/ot/opentracing_driver_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,12 @@ void OpenTracingSpan::log(SystemTime timestamp, const std::string& event) {
finish_options_.log_records.emplace_back(std::move(record));
}

void OpenTracingSpan::setBaggage(const std::string& key, const std::string& value) {
span_->SetBaggageItem(key, value);
}

std::string OpenTracingSpan::getBaggage(const std::string& key) { return span_->BaggageItem(key); }
matthewgrossman marked this conversation as resolved.
Show resolved Hide resolved

void OpenTracingSpan::injectContext(Http::RequestHeaderMap& request_headers) {
if (driver_.propagationMode() == OpenTracingDriver::PropagationMode::SingleHeader) {
// Inject the span context using Envoy's single-header format.
Expand Down
2 changes: 2 additions & 0 deletions source/extensions/tracers/common/ot/opentracing_driver_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ class OpenTracingSpan : public Tracing::Span, Logger::Loggable<Logger::Id::traci
Tracing::SpanPtr spawnChild(const Tracing::Config& config, const std::string& name,
SystemTime start_time) override;
void setSampled(bool) override;
std::string getBaggage(const std::string& key) override;
void setBaggage(const std::string& key, const std::string& value) override;

private:
OpenTracingDriver& driver_;
Expand Down
5 changes: 5 additions & 0 deletions source/extensions/tracers/opencensus/opencensus_tracer_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ class Span : public Tracing::Span {
void log(SystemTime timestamp, const std::string& event) override;
void finishSpan() override;
void injectContext(Http::RequestHeaderMap& request_headers) override;
void setBaggage(const std::string&, const std::string&) override;
std::string getBaggage(const std::string&) override;
Tracing::SpanPtr spawnChild(const Tracing::Config& config, const std::string& name,
SystemTime start_time) override;
void setSampled(bool sampled) override;
Expand Down Expand Up @@ -192,6 +194,9 @@ void Span::log(SystemTime /*timestamp*/, const std::string& event) {
// timestamp is ignored.
span_.AddAnnotation(event);
}
void Span::setBaggage(const std::string&, const std::string&) {}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.");

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


void Span::finishSpan() { span_.End(); }

Expand Down
3 changes: 3 additions & 0 deletions source/extensions/tracers/xray/tracer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,9 @@ class Span : public Tracing::Span, Logger::Loggable<Logger::Id::config> {
*/
void log(Envoy::SystemTime, const std::string&) override {}

void setBaggage(const std::string&, const std::string&) override {}
std::string getBaggage(const std::string&) override { return std::string(); }
matthewgrossman marked this conversation as resolved.
Show resolved Hide resolved

/**
* Creates a child span.
* In X-Ray terms this creates a sub-segment and sets its parent ID to the current span's ID.
Expand Down
4 changes: 4 additions & 0 deletions source/extensions/tracers/zipkin/zipkin_tracer_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ void ZipkinSpan::log(SystemTime timestamp, const std::string& event) {
span_.log(timestamp, event);
}

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

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

void ZipkinSpan::injectContext(Http::RequestHeaderMap& request_headers) {
// Set the trace-id and span-id headers properly, based on the newly-created span structure.
request_headers.setReferenceKey(ZipkinCoreConstants::get().X_B3_TRACE_ID,
Expand Down
2 changes: 2 additions & 0 deletions source/extensions/tracers/zipkin/zipkin_tracer_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ class ZipkinSpan : public Tracing::Span {
SystemTime start_time) override;

void setSampled(bool sampled) override;
void setBaggage(const std::string&, const std::string&) override;
std::string getBaggage(const std::string&) override;

/**
* @return a reference to the Zipkin::Span object.
Expand Down
11 changes: 11 additions & 0 deletions test/extensions/tracers/lightstep/lightstep_tracer_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,17 @@ TEST_F(LightStepDriverTest, SpawnChild) {
EXPECT_FALSE(base2_context.empty());
}

TEST_F(LightStepDriverTest, GetAndSetBaggage) {
setupValidDriver();
Tracing::SpanPtr span = driver_->startSpan(config_, request_headers_, operation_name_,
start_time_, {Tracing::Reason::Sampling, true});

std::string key = "key1";
std::string value = "value1";
span->setBaggage(key, value);
EXPECT_EQ(span->getBaggage(key), value);
}

} // namespace
} // namespace Lightstep
} // namespace Tracers
Expand Down
2 changes: 2 additions & 0 deletions test/mocks/tracing/mocks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ class MockSpan : public Span {
MOCK_METHOD(void, finishSpan, ());
MOCK_METHOD(void, injectContext, (Http::RequestHeaderMap & request_headers));
MOCK_METHOD(void, setSampled, (const bool sampled));
MOCK_METHOD(void, setBaggage, (const std::string& key, const std::string& value));
MOCK_METHOD(std::string, getBaggage, (const std::string& key));

SpanPtr spawnChild(const Config& config, const std::string& name,
SystemTime start_time) override {
Expand Down