Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Add Resource to Span #174

Merged

Conversation

sjkaris
Copy link

@sjkaris sjkaris commented Jan 23, 2019

As discussed in the wider meeting, adds Resource information to individual spans.

@sjkaris
Copy link
Author

sjkaris commented Jan 23, 2019

fixes #169

@@ -290,6 +291,11 @@ message Span {
// Status.Ok (code = 0).
Status status = 11;

// An optional resource that this span describes. If not set, this span
// should be part of a batch that does include the span information, unless resource

Choose a reason for hiding this comment

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

"that does include the span information" or "that does include the resource information"?

Copy link
Author

Choose a reason for hiding this comment

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

ah you are right, should be resource. -- fixing

Copy link
Contributor

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -290,6 +291,11 @@ message Span {
// Status.Ok (code = 0).
Status status = 11;

// An optional resource that this span describes. If not set, this span
Copy link
Contributor

Choose a reason for hiding this comment

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

span doesn't describe resource. May be this should be rephrased to something like "An optional resource to which this span is associated with".

Copy link
Author

Choose a reason for hiding this comment

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

Reworded. Please take another look

Copy link
Contributor

@rghetia rghetia left a comment

Choose a reason for hiding this comment

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

LGTM

@bogdandrutu bogdandrutu merged commit fb7c17e into census-instrumentation:master Jan 25, 2019
@sjkaris sjkaris deleted the add-resource-to-span branch January 25, 2019 19:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants