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

Add Express Trace Plugin #363

Merged
merged 6 commits into from
Feb 21, 2017
Merged

Add Express Trace Plugin #363

merged 6 commits into from
Feb 21, 2017

Conversation

kjin
Copy link
Contributor

@kjin kjin commented Feb 7, 2017

  • Add Express Trace Plugin
  • Remove Express Hook

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 7, 2017
@kjin
Copy link
Contributor Author

kjin commented Feb 7, 2017

Dependent on #366 - PTAL at that first

ofrobots
ofrobots reviewed Feb 7, 2017
View reviewed changes
src/plugins/plugin-express.js
@@ -0,0 +1,70 @@
'use strict';

This comment was marked as spam.

Sign in to view

matthewloring
matthewloring reviewed Feb 7, 2017
View reviewed changes
Copy link
Contributor

@matthewloring matthewloring left a comment

Choose a reason for hiding this comment

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

Looks good structurally, we'll have to look into the failing tests.

src/plugins/plugin-express.js Outdated

module.exports = [
{ versions: SUPPORTED_VERSIONS, patch: patchModuleRoot }
];

This comment was marked as spam.

Sign in to view

@kjin
Copy link
Contributor Author

kjin commented Feb 7, 2017

Test failures are due to us no longer unpatching. So this is blocked on stop (and related tests) being removed completely.

@kjin kjin force-pushed the express-plugin branch 3 times, most recently from 8416c51 to 62ab860 Compare February 16, 2017 01:09
@kjin
Copy link
Contributor Author

kjin commented Feb 16, 2017

CI failures are due to differences in context propagation in mocha in Node v0.12. This is apparent if we add

if (this.isActive() && config.forceNewAgent_) {
  agent.stop();
}

to the public agent's start method, and is the same reason why #378 was also failing in Node v0.12.

While the fix is small, I think this may a compelling reason to drop support for v0.12. @ofrobots @matthewloring What are your thoughts on this?

@kjin
Copy link
Contributor Author

kjin commented Feb 18, 2017

Blocked on #388.

matthewloring
matthewloring suggested changes Feb 18, 2017
View reviewed changes
src/plugins/plugin-express.js
function middleware(req, res, next) {
var options = {
name: req.path,
traceContext: req.get(api.constants.TRACE_CONTEXT_HEADER_NAME),

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@matthewloring matthewloring added this to the Beta milestone Feb 20, 2017
@kjin kjin merged commit da804af into googleapis:master Feb 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ofrobots ofrobots ofrobots left review comments

@matthewloring matthewloring matthewloring approved these changes

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