feat(sdk): enable xray #5025
feat(sdk): enable xray #5025
Conversation
```
FAIL test/target-tf-aws/api.test.ts > api with GET routes with different prefix
Error: Failed to bundle function: Error: Build failed with 1 error:
api-onrequest-c5395e41_c8f26cae.js:2:24: ERROR: Could not resolve "aws-xray-sdk"
❯ createBundle src/shared/bundling.ts:54:11
❯ new Function src/target-tf-aws/function.ts:84:32
❯ Api.addInflightHandler src/target-tf-aws/api.ts:258:12
❯ Api.getExistingOrAddInflightHandler src/target-tf-aws/api.ts:219:17
❯ Api.addHandler src/target-tf-aws/api.ts:201:19
❯ Api.httpRequests src/target-tf-aws/api.ts:78:21
❯ Api.get src/target-tf-aws/api.ts:100:10
❯ test/target-tf-aws/api.test.ts:83:7
81| const inflight2 = Testing.makeHandler(app, "Handler2", INFLIGHT_CODE);
82|
83| api.get("/hello/foo", inflight);
| ^
84| api.get("/foo/bar", inflight2);
```
|
|
||
| lines.push('"use strict";'); | ||
| lines.push(`const AWSXRay = require('aws-xray-sdk');`); | ||
| lines.push(`AWSXRay.captureHTTPsGlobal(require('http'));`); |
There was a problem hiding this comment.
Would this also capture fetch which we are using to implement the wing http module?
There was a problem hiding this comment.
hm, good point - aws/aws-xray-sdk-node#531 - no, it's not. They apparently recommend to use some open telemetry sdk. Will check.
There was a problem hiding this comment.
looks interesting https://aws-otel.github.io/
Also, seems to be less intrusive and more generic. Will see if that's really just pluggable via ENV, layer and permissions.
There was a problem hiding this comment.
yes! is it possible to use OT for everything instead of directly xray? that would be a much better and general solution.
| lines.push(`const AWSXRay = require('aws-xray-sdk');`); | ||
| lines.push(`AWSXRay.captureHTTPsGlobal(require('http'));`); | ||
| lines.push("exports.handler = async function(event) {"); | ||
| lines.push(` return await (${inflightClient}).handle(event);`); |
There was a problem hiding this comment.
| lines.push(` return await (${inflightClient}).handle(event);`); | |
| lines.push(` return (${inflightClient}).handle(event);`); |
Not needed
| * @param handler IFunctionHandler | ||
| * @returns the function code lines as strings | ||
| */ | ||
| protected _getCodeLines(handler: IFunctionHandler): string[] { |
|
Console preview environment is available at https://wing-console-pr-5025.fly.dev 🚀 Last Updated (UTC) 2023-11-22 08:55 |
BenchmarksComparison to Baseline 🟥⬜⬜⬜⬜⬜⬜⬜⬜⬜
⬜ Within 1.5 standard deviations Benchmarks may vary outside of normal expectations, especially when running in GitHub Actions CI. Results
Last Updated (UTC) 2023-11-22 09:02 |
|
did some research yesterday, it's possible to add open telemetry without changes and get some basic instrumentation going.
However, the segments for AWS SDK calls are missing. It looks like we're bundling the There were attempts to exclude it from bundling already #4152 which led to drastically increased cold starts #4125 - Perhaps there's a way to get around the performance issues. Will give it a quick try. btw: that's a case where #3410 would be super helpful to quickly iterate and upstream once it's working. |
|
oh, and there's a tax on the cold start times for adding the otel agent. Roughly in the range of 0.5 - 1s |
|
for reference, here's a recent issue in the aws skd v3 repo talking about the cold start issues with unbundled sdk. ~ 1s added at 1024mb aws/aws-sdk-js-v3#5382 - so I'm assuming it gets way worse with less memory. And they recommend to bundle 🤡 |
Oof... any way to avoid this? |
just double checked to be sure: with open telemetry ~ 1.3 seconds init
same but plain lambda ~ 350 ms init also, this is just a single sample but the execution time of the function itself appears to be slower with otel. But this could be just a coincident. perhaps bumping up the memory could speed things up a bit. But doesn't seem to be a good strategy. |
no, there's no difference in actual runtime. Only in terms of cold start / init.
No, doubling to
Deployed a little test application and customized it with a platform which uses the default aws otel layer with config, no code changes. While it picks up the general flow, it looks a bit confusing.
And that's the related trace
|
There's a pretty good blog post about this by https://twitter.com/astuyve |
|
some more links around the aws open telemetry stuff
|
|
re: fetch for xray - there's a pretty nice workaround posted here by @RaphaelManke aws-powertools/powertools-lambda-typescript#1619 |
|
Hi, This PR has not seen activity in 20 days. Therefore, we are marking the PR as stale for now. It will be closed after 7 days. |
|
😞 |
|
for wing cloud, we're using a custom platform to define the open telemetry config, which works ok. As said above, it adds substantial cold start times though. So, perhaps an opt-in option for functions would work? |
|
Hi, This PR has not seen activity in 20 days. Therefore, we are marking the PR as stale for now. It will be closed after 7 days. |
This aims to do the following things:
For
http, I haven't found the right way to inject the necessary code. Tests are failing with something like this:Also, I'm not quite sure if the current approach for
httpinstrumentation is the right way. I think in another project I brought in the xray sdk via a Lambda layer and excluded it from bundling. But I don't think this would help with the failing tests. Thoughts / ideas?Related to #4830
Checklist
pr/e2e-fulllabel if this feature requires end-to-end testingBy submitting this pull request, I confirm that my contribution is made under the terms of the Wing Cloud Contribution License.