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.

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

Express middleware does not return trace header in response #30

Open
corydolphin opened this issue Apr 11, 2018 · 10 comments
Open

Express middleware does not return trace header in response #30

corydolphin opened this issue Apr 11, 2018 · 10 comments
Assignees

Comments

@corydolphin
Copy link

Hello,

The ExpressJs middleware does not set the X-Amz-TraceId header on the response. The Java ServletFilter does. Seems like this would be universally useful, please add.

@haotianw465
Copy link
Contributor

Thank you for the feedback. We will add this functionality in the next release. In the meantime we would also like to know more about the use case of getting trace header on the response if you would like to share.

@xiananfan
Copy link

Hi @haotianw465 , I have encountered the same issue, when using with Express/Connect, the res.req might not be set. The mw_utils.js assumes res.req is always there, and uses the res.req.headers field. This leads to runtime error: TypeError: Cannot read property 'headers' of undefined. Issue code. I am also more inclined to mark this issue as a bug than just enhancement.

Sample error stack trace I got:

2018-07-06T20:15:10.191Z	47549d19-8159-11e8-aa67-0b6cabc0bb8a	TypeError: Cannot read property 'headers' of undefined
at Object.resolveSampling (/var/task/node_modules/aws-xray-sdk-core/lib/middleware/mw_utils.js:90:53)
at open (/var/task/node_modules/aws-xray-sdk-express/lib/express_mw.js:40:15)
at call (/var/task/node_modules/connect/index.js:239:7)
at next (/var/task/node_modules/connect/index.js:183:5)
at /var/task/node_modules/aws-serverless-express/middleware.js:22:5
at call (/var/task/node_modules/connect/index.js:239:7)
at next (/var/task/node_modules/connect/index.js:183:5)
at Function.handle (/var/task/node_modules/connect/index.js:186:3)
at Server.app (/var/task/node_modules/connect/index.js:51:37)
at emitTwo (events.js:126:13)

To give you some context, I am using expressJS framework to build a microservice, and then also uses the aws-serverless-express middleware to run the express microservice as a serverless API. In other words, the code is executed in AWS lambda. And above stacktrace is from cloudwatch.

@haotianw465 haotianw465 changed the title Express middleware does not set response headers Express middleware does not return trace header in response Jul 6, 2018
@haotianw465
Copy link
Contributor

Hi @xiananfan, sorry to hear the issue you encountered. This issue is for tracking a different use case where the express middleware provided from the X-Ray SDK should set the trace header with key X-Amz-TraceId in the response body.

The issue you mentioned is for supporting running expressJS in Lambda. It doesn't work out of box is because Lambda container sends segment for function invocation so the SDK does special handling to cooperate Lambda and it doesn't handle the function being a web app serving proxied request.

Right now this support is in our backlog. We will look into this item as soon as we can. Please stay tuned and thank you for your patience. Feel free to open a separate issue and pull requests are always welcome.

@xiananfan
Copy link

Hi @haotianw465, thank you for the response. Can you give more contexts on what you mean by "because Lambda container sends segment for function invocation so the SDK does special handling to cooperate Lambda and it doesn't handle the function being a web app serving proxied request"? Can you link me to the "special handling" code in SDK? I am happy to work on this issue if you can point me to the right direction on what are the likely affected components?

@haotianw465
Copy link
Contributor

Sure. The SDK has a different way of managing segment context when running in a AWS Lambda container. You can see the logic here: https://github.com/aws/aws-xray-sdk-node/blob/master/packages/core/lib/env/aws_lambda.js.

Originally the SDK fits these two use cases: 1. For an express app, the SDK will create a segment for each request/response cycle and the intermediate remote calls or local computations happen in that cycle will be captured as subsegments and attached to the parent segment. 2. For a Lambda function, the Lambda container will create a segment for each function invocation and send it independently. The SDK will create a facade segment for that invocation to keep the structure intact. Intermediate remote calls or local computations happen in that invocation will be captured as subsegments and attached to the facade segment. The SDK will stream out those subsegments and leave the facade segment until it is later cleaned up. The X-Ray back-end will re-assemble the real segment from Lambda container and subsegments from function code.

When running an express app inside a Lambda function, there will be a conflict of Lambda segment and express middleware segment both try to represent a root component in the microservice architecture (and further the service graph in AWS X-Ray console). The SDK should support this use case properly. We have a backlog item to work together with Lambda team to provide better customer experience for such use case. But we can't guarantee any further details or ETA at this point.

Please let me know if you have any additional concerns. There are some further discussion about workaround but in python aws/aws-xray-sdk-python#2. Feel free to share any thought at either place.

@xiananfan
Copy link

xiananfan commented Jul 6, 2018

Hi @haotianw465, thank you for the explanation, this makes more sense to me now.

So here is my thought:
My guess is when x-ray was initially designed, it was not expecting a classic web server app will be running inside a lambda, and this causes the SDK to start differentiate these 2 uses cases. However, with the introduction of aws-serverless-express, it's likely only more classic web app will be able to move towards serverless based computation instead of stick with traditional EC2 hosts.

So, why not just treat lambda as another computation platform like EC2, ElasticBeanstalk and ECS? In AWS documentations, there is a section about plugins, for EC2, ECS and Beanstalk. Why not creating a plugin for lambda as well? In other words, disable the special handling by default, and let the user decide whether to enable creating segment automatically or manually by plugin? Maybe during the transitional phase, still support "special handling", but start support plugin based segment initialization?

@haotianw465
Copy link
Contributor

Thank you for the valuable suggestions. We will definitely bring these feedback to Lambda team and work together to provide a better integrated experience in the very near future. I've also created an issue #45 to track the feature so the current issue thread can focus on the response header. Please feel free to leave any additional comment on the new created issue.

@ryan-roemer
Copy link

ryan-roemer commented Jul 27, 2018

This is completely blocking our ability to trace Knex + MySQL calls (we're using serverless-http and express, but same thing -- I'm debugging and seeing the facade segment clobber the express middleware one).

Does anyone have an hacks or patches that I can apply right now to have AWSXRay.getSegment() always use the Express MW segment and never the lambda facade one?

Thanks!

@sergio-dreamcode
Copy link

Hello I see the issue is still open, currently I use X-Ray SDK in a NodeJs docker container that runs in EKS and all requests go to the containers via a Classic Load Balancer which does not support tracing. Although the traces are sent to AWS via the X-Ray Daemon and I am able to visualize them in AWS, the response does not have the Trace ID header.

I was reviewing library's code and in mwUtils.traceRequestResponseCycle trace id header is is added to the response when sampling is ? but it expects the request to have the trace id header. Is it possible to return the Trace ID as a response header in Express in a scenario like this? i.e. Classic Load Balancer -> Docker container.

@willarmiros
Copy link
Contributor

willarmiros commented Sep 29, 2020

Hi @sergio-dreamcode would just returning the trace ID in a response header be sufficient for your use case? We try to minimize passing around the trace header to not overload other AWS Services that do process it. I can discuss with my team to return the header by default, and not just when there's a ? sample decision.

Also, can you elaborate on your classic load balancer use case? Specifically, how will you use the returned trace header in your load balancer to include it in your service map?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants