Skip to content
This repository was archived by the owner on Jan 21, 2026. It is now read-only.

Set response trace context based on that of incoming requests#463

Merged
kjin merged 4 commits intogoogleapis:masterfrom
kjin:response-context
Apr 14, 2017
Merged

Set response trace context based on that of incoming requests#463
kjin merged 4 commits intogoogleapis:masterfrom
kjin:response-context

Conversation

@kjin
Copy link
Copy Markdown
Contributor

@kjin kjin commented Apr 10, 2017

Previously, plugins that patched web frameworks would add in response headers the trace context corresponding to the locally-created root span. This should actually be set to the trace context of the incoming request (modified based on whether this request was traced).

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 10, 2017
@kjin kjin force-pushed the response-context branch from 7f24b87 to b0d0ec8 Compare April 10, 2017 23:52
ofrobots
ofrobots suggested changes Apr 10, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

I don't think the two commits are individually landable because of the API change in trace-api.js. It would be preferable if the test improvements were completely standalone or were done in a follow-on.

Comment thread src/plugins/plugin-express.js Outdated
};
api.runInRootSpan(options, function(rootSpan) {
// Set response trace context.
var outgoingTraceContext =

This comment was marked as spam.

Sign in to view

Comment thread src/plugins/plugin-hapi.js
// we use the path part of the url as the span name and add the full
// url as a label
// req.path would be more desirable but is not set at the time our middlewear runs.
// req.path would be more desirable but is not set at the time our middleware runs.

This comment was marked as spam.

Sign in to view

Comment thread src/trace-api.js Outdated
skipFrames + 1);
return new RootSpan(api.agent_, rootContext);
}
} No newline at end of file

This comment was marked as spam.

Sign in to view

Comment thread src/util.js Outdated
return null;
}
// We assume that if no value is set for options, we treat the incoming
// context as if it had options set to 1.

This comment was marked as spam.

Sign in to view

@kjin
Copy link
Copy Markdown
Contributor Author

kjin commented Apr 10, 2017

@ofrobots I haven't done any test refactoring in this PR, what do you mean by that the "two commits aren't individually landable"?

@ofrobots
Copy link
Copy Markdown
Contributor

ofrobots commented Apr 10, 2017

'improvements' removes getOutgoingTraceContext and the other commit adds it back in. I would suggest squashing these changes (perhaps at landing time).

@kjin
Copy link
Copy Markdown
Contributor Author

kjin commented Apr 11, 2017

Oh I see - I was planning to squash these commits.

@kjin kjin force-pushed the response-context branch 6 times, most recently from b28f75b to 7f50aad Compare April 11, 2017 02:02
@kjin
Copy link
Copy Markdown
Contributor Author

kjin commented Apr 11, 2017

@GoogleCloudPlatform/node-team CI passed, PTAL

@ofrobots ofrobots mentioned this pull request Apr 11, 2017
Merged
matthewloring
matthewloring suggested changes Apr 11, 2017
View reviewed changes
Comment thread src/trace-api.js
var traceContext = this.agent_.parseContextFromHeader(incomingTraceContext);
if (!traceContext) {
return '';
}

This comment was marked as spam.

Sign in to view

Comment thread src/trace-api.js Outdated
enhancedDatabaseReportingEnabled: function() { return false; },
runInRootSpan: function(opts, fn) { return fn(null); },
createChildSpan: function(opts) { return null; },
getResponseTraceContext: function(context, traced) { return null; },

This comment was marked as spam.

Sign in to view

Comment thread src/util.js Outdated
traceId: matches[1],
spanId: matches[2],
options: Number(matches[3])
options: typeof(matches[3]) === 'undefined' ? 0 : Number(matches[3])

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@kjin kjin force-pushed the response-context branch 4 times, most recently from 6841c6b to 2743047 Compare April 13, 2017 18:15
@kjin kjin force-pushed the response-context branch from 2743047 to 0f870ae Compare April 13, 2017 18:18
@kjin kjin changed the title Set response trace context based that of incoming requests Set response trace context based on that of incoming requests Apr 13, 2017
@kjin kjin merged commit 42de95a into googleapis:master Apr 14, 2017
yoshi-automation added a commit that referenced this pull request Apr 1, 2020
@yoshi-automation
fix: update template files for Node.js libraries (#463)
7c72697 
googleapis/synthtool@9982024
commit 99820243d348191bc9c634f2b48ddf65096285ed
Author: Alexander Fenster <[email protected]>
Date:   Tue Mar 31 11:56:27 2020 -0700

    fix: update template files for Node.js libraries (#463)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DominicKramer DominicKramer Awaiting requested review from DominicKramer

2 more reviewers

@ofrobots ofrobots ofrobots approved these changes

@matthewloring matthewloring matthewloring approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kjin @ofrobots @matthewloring @googlebot