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 direct support for a span to spawn a child #932

Merged
merged 11 commits into from
May 18, 2017
Merged

tracing: add direct support for a span to spawn a child #932

merged 11 commits into from
May 18, 2017

Conversation

goaway
Copy link
Contributor

@goaway goaway commented May 9, 2017

No description provided.

@goaway
Copy link
Contributor Author

goaway commented May 9, 2017

needs test

@goaway goaway mentioned this pull request May 9, 2017
Merged
RomanDzhabarov
RomanDzhabarov reviewed May 16, 2017
View reviewed changes
source/common/tracing/http_tracer_impl.h Outdated
@@ -87,6 +87,11 @@ class NullSpan : public Tracing::Span {
// Tracing::Span
void setTag(const std::string&, const std::string&) override {}
void finishSpan() override {}
SpanPtr spawnChild(const std::string&, SystemTime) override {
SpanPtr nullSpan;
Copy link
Member

Choose a reason for hiding this comment

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

very minor, SpanPtr nullSpan(new NullSpan())

source/common/tracing/lightstep_tracer_impl.cc Outdated

void LightStepSpan::finishSpan() { span_.Finish(); }

void LightStepSpan::setTag(const std::string& name, const std::string& value) {
span_.SetTag(name, value);
}

SpanPtr LightStepSpan::spawnChild(const std::string& name, SystemTime start_time) {
SpanPtr child_span;
Copy link
Member

Choose a reason for hiding this comment

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

same here.

source/common/tracing/lightstep_tracer_impl.cc Outdated
SpanPtr child_span;
lightstep::Span ls_span = tracer_.StartSpan(
name, {lightstep::ChildOf(span_.context()), lightstep::StartTimestamp(start_time)});
child_span.reset(new LightStepSpan(ls_span, tracer_));
Copy link
Member

Choose a reason for hiding this comment

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

nit, you can move child_span declaration right in here.

source/common/tracing/lightstep_tracer_impl.cc Outdated
name, {lightstep::ChildOf(span_.context()), lightstep::StartTimestamp(start_time)});
child_span.reset(new LightStepSpan(ls_span, tracer_));

return std::move(child_span);
Copy link
Member

Choose a reason for hiding this comment

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

should not we be able to just return chlid_span

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can we? this seems to be a common pattern adopted elsewhere, but I'm not familiar enough with the ramifications.

Copy link
Member

Choose a reason for hiding this comment

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

this should be possible as child_span is actually SpanPtr type.

include/envoy/tracing/http_tracer.h Outdated
class Span {
public:
virtual ~Span() {}

virtual void setTag(const std::string& name, const std::string& value) PURE;
virtual void finishSpan() PURE;
virtual void injectContext(Http::HeaderMap& request_headers) PURE;
Copy link
Member

Choose a reason for hiding this comment

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

what about overrides for zipkin spans?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

RomanDzhabarov
RomanDzhabarov reviewed May 16, 2017
View reviewed changes
source/common/tracing/http_tracer_impl.h Outdated
@@ -88,6 +88,12 @@ class NullSpan : public Tracing::Span {
// Tracing::Span
void setTag(const std::string&, const std::string&) override {}
void finishSpan() override {}
void injectContext(Http::HeaderMap&) override {}
SpanPtr spawnChild(const std::string&, SystemTime) override {
Copy link
Member

Choose a reason for hiding this comment

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

null span had 0 coverage, can you also update your PR to include that?
and make sure that we have full coverage of the new code as well

RomanDzhabarov
RomanDzhabarov reviewed May 16, 2017
View reviewed changes
source/common/tracing/zipkin/zipkin_tracer_impl.cc Outdated
Tracing::SpanPtr ZipkinSpan::spawnChild(const std::string& name, SystemTime start_time) {
Tracing::SpanPtr child_span;
SpanContext context;
new SpanContext(span_);
Copy link
Member

Choose a reason for hiding this comment

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

something is broken here :-)

RomanDzhabarov
RomanDzhabarov reviewed May 16, 2017
View reviewed changes
source/common/tracing/zipkin/zipkin_tracer_impl.cc Outdated

ZipkinSpanPtr active_span;
active_span.reset(new ZipkinSpan(*new_zipkin_span));
active_span.reset(new ZipkinSpan(*new_zipkin_span, tracer));
Copy link
Member

Choose a reason for hiding this comment

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

nit: ZipkinSpanPtr active_span(new ZipkinSpan(*new_zipkin_span, tracer))

Copy link
Member

Choose a reason for hiding this comment

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

i think this is still a valid comment :)

@goaway
Copy link
Contributor Author

goaway commented May 17, 2017

cc @fabolive

RomanDzhabarov
RomanDzhabarov reviewed May 17, 2017
View reviewed changes
include/envoy/tracing/http_tracer.h Outdated
class Span;

typedef std::unique_ptr<Span> SpanPtr;

class Span {
public:
virtual ~Span() {}

virtual void setTag(const std::string& name, const std::string& value) PURE;
Copy link
Member

@RomanDzhabarov RomanDzhabarov May 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

comments here in all these methods, sorry I've not put it in the first place

mattklein123
mattklein123 reviewed May 17, 2017
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

in general lgtm. Will let @RomanDzhabarov do the review.

RomanDzhabarov
RomanDzhabarov reviewed May 17, 2017
View reviewed changes
source/common/tracing/zipkin/zipkin_tracer_impl.cc Outdated
}

Tracing::SpanPtr ZipkinSpan::spawnChild(const std::string& name, SystemTime start_time) {
std::unique_ptr<SpanContext> context{new SpanContext(span_)};
Copy link
Member

Choose a reason for hiding this comment

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

this can be allocated on a stack. SpanContext context(span_);

RomanDzhabarov
RomanDzhabarov reviewed May 18, 2017
View reviewed changes
source/common/tracing/zipkin/zipkin_tracer_impl.cc Outdated
// Set the ot-span-context header with the new context.
SpanContext new_span_context(*new_zipkin_span);
request_headers.insertOtSpanContext().value(new_span_context.serializeToString());

ZipkinSpanPtr active_span;
Copy link
Member

Choose a reason for hiding this comment

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

minor, ZipkinSpanPtr active_span(new ZipkinSpan(*new_zipkin_span, tracer));
and remove reset.

RomanDzhabarov
RomanDzhabarov previously approved these changes May 18, 2017
View reviewed changes
Copy link
Member

@RomanDzhabarov RomanDzhabarov left a comment

Choose a reason for hiding this comment

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

Small comment and +1

@goaway goaway merged commit 43c59df into master May 18, 2017
@RomanDzhabarov RomanDzhabarov deleted the spawn-child branch May 24, 2017 05:53
@mattklein123 mattklein123 mentioned this pull request Jun 8, 2017
Closed
rshriram pushed a commit to rshriram/envoy that referenced this pull request Oct 30, 2018
@rkpagadala
add envoyproxy as dependency (envoyproxy#932)
7ba0bc7 
Automatic merge from submit-queue.

add envoyproxy as dependency

**What this PR does / why we need it**:
Add envoy proxy as dependency in istio.deps, after this PR is merged envoy proxy will be be updated once every week.

**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
```
jpsim pushed a commit that referenced this pull request Nov 28, 2022
@keith @jpsim
Revert "Swift: Workaround Swift 5.1 swiftinterface bug (#914)" (#932)
9ed4f24 
This reverts ef8a2fc2f388853b7db3498cce43e32a813c8428 and e333c485103c14983d2465f0befad8134f49a229

These are no longer required since we now build with Swift 5.2

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
@keith @jpsim
Revert "Swift: Workaround Swift 5.1 swiftinterface bug (#914)" (#932)
180fa90 
This reverts ef8a2fc2f388853b7db3498cce43e32a813c8428 and e333c485103c14983d2465f0befad8134f49a229

These are no longer required since we now build with Swift 5.2

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@RomanDzhabarov RomanDzhabarov RomanDzhabarov left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@goaway @mattklein123 @RomanDzhabarov