feat: multiple onRequest handlers #1863

webbdays · 2024-05-05T06:15:47Z

Summary:
multiple onRequest handlers

Issue Reference(s):
Fixes #1271

Build & Testing:

I ran cargo test successfully.
I have run ./lint.sh --mode=fix to fix all linting issues raised by ./lint.sh --mode=check.

Checklist:

I have added relevant unit & integration tests.
I have updated the documentation accordingly.
I have performed a self-review of my code.
[yes ] PR follows the naming convention of <type>(<optional scope>): <title>

codecov · 2024-05-05T09:37:03Z

Codecov Report

Attention: Patch coverage is 70.81340% with 61 lines in your changes are missing coverage. Please review.

Project coverage is 82.53%. Comparing base (e6d8e38) to head (539c98f).
Report is 1 commits behind head on main.

Files	Patch %	Lines
src/core/grpc/request.rs	0.00%	27 Missing ⚠️
tailcall-cloudflare/src/http.rs	0.00%	17 Missing ⚠️
tailcall-aws-lambda/src/http.rs	0.00%	12 Missing ⚠️
src/core/http/mod.rs	0.00%	3 Missing ⚠️
src/cli/javascript/request_filter.rs	93.33%	1 Missing ⚠️
src/core/data_loader/data_loader.rs	98.86%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1863      +/-   ##
==========================================
- Coverage   82.70%   82.53%   -0.17%     
==========================================
  Files         177      178       +1     
  Lines       18051    18215     +164     
==========================================
+ Hits        14929    15034     +105     
- Misses       3122     3181      +59

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-05-12T12:03:34Z

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

tusharmath · 2024-05-15T12:14:23Z

@webbdays Can you please resolve the conflicts.

tusharmath · 2024-05-15T12:15:00Z

Whenever you are ready, just mark it as ready to review.

webbdays · 2024-05-16T04:54:39Z

ok.

github-actions · 2024-05-17T03:18:50Z

Bencher

Report	Sat, May 18, 2024 at 13:28:55 UTC
Project	tailcall
Branch	1863/merge
Testbed	benchmarking-runner

🚨 1 ALERT: Threshold Boundary Limit exceeded!

Benchmark	Measure (units)	View	Value	Lower Boundary	Upper Boundary
input/vars.missing	Latency (nanoseconds (ns))	🚨 (view plot \| view alert)	11.72 (+36.35%)		11.14 (105.23%)

Click to view all benchmark results

Benchmark	Latency	Latency Results nanoseconds (ns) \| (Δ%)	Latency Upper Boundary nanoseconds (ns) \| (%)
group_by	✅ (view plot)	529.69 (-11.44%)	650.03 (81.49%)
input/args.missing	✅ (view plot)	22.95 (-7.36%)	27.36 (83.87%)
input/args.nested.existing	✅ (view plot)	41.77 (-19.53%)	65.97 (63.31%)
input/args.nested.missing	✅ (view plot)	37.49 (-2.69%)	40.61 (92.32%)
input/args.root	✅ (view plot)	38.54 (-20.48%)	62.48 (61.69%)
input/headers.existing	✅ (view plot)	30.48 (-4.05%)	33.48 (91.03%)
input/headers.missing	✅ (view plot)	29.20 (-5.08%)	33.36 (87.52%)
input/value.missing	✅ (view plot)	23.06 (-2.03%)	25.35 (90.98%)
input/value.nested.existing	✅ (view plot)	41.95 (+1.07%)	43.95 (95.44%)
input/value.nested.missing	✅ (view plot)	37.22 (+2.20%)	38.36 (97.04%)
input/value.root	✅ (view plot)	37.59 (-2.34%)	40.38 (93.09%)
input/vars.existing	✅ (view plot)	7.43 (-8.89%)	9.06 (82.07%)
input/vars.missing	🚨 (view plot \| view alert)	11.72 (+36.35%)	11.14 (105.23%)
test_batched_body	✅ (view plot)	2,695.80 (-99.68%)	2,642,500.31 (0.10%)
test_batched_body #2	✅ (view plot)	1,647,800.00 (-5.20%)	1,885,525.26 (87.39%)
test_data_loader	✅ (view plot)	465,820.00 (-1.37%)	490,230.95 (95.02%)
test_handle_request	✅ (view plot)	157,350.00 (-7.51%)	183,106.20 (85.93%)
with_mustache_expressions	✅ (view plot)	1,167.90 (-0.31%)	1,235.71 (94.51%)
with_mustache_literal	✅ (view plot)	729.70 (+0.78%)	772.16 (94.50%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

src/core/grpc/data_loader.rs

src/core/graphql/data_loader.rs

tusharmath · 2024-05-18T13:20:40Z

src/core/grpc/request.rs

+                    headers,
+                    body: Bytes::default(),
+                }),
+            }


Can we just call execute here? There is quite a bit of code duplication.

tusharmath · 2024-05-18T13:21:25Z

src/core/http/data_loader.rs

@@ -65,6 +65,7 @@ impl Loader<DataLoaderRequest> for HttpDataLoader {
    async fn load(
        &self,
        keys: &[DataLoaderRequest],
+        http_filter: HttpFilter,


Store the filter in HttpDataLoader as a field

Can you please explain more. Where is it used later in the code?

tusharmath · 2024-05-18T13:21:41Z

src/core/http/filter.rs

+#[derive(Default, Clone, Debug)]
+/// User can configure the filter/interceptor
+/// for the http requests.
+pub struct HttpFilter {
+    pub on_request: Option<String>,
+}


can delete this file. Move it to http module.

tusharmath · 2024-05-18T13:23:28Z

src/core/lambda/io.rs

                    let req = req_template.to_request(&ctx)?;
                    let is_get = req.method() == reqwest::Method::GET;

                    let res = if is_get && ctx.request_ctx.is_batching_enabled() {
                        let data_loader: Option<&DataLoader<DataLoaderRequest, HttpDataLoader>> =
                            dl_id.and_then(|index| ctx.request_ctx.http_data_loaders.get(index.0));
-                        execute_request_with_dl(&ctx, req, data_loader).await?
+                        execute_request_with_dl(&ctx, req, data_loader, http_filter.clone()).await?


This is hot path. Let's not perform any clones here. Pass the filter as a reference.

tusharmath · 2024-05-18T13:24:01Z

src/core/lambda/io.rs

@@ -27,6 +27,7 @@ pub enum IO {
        req_template: http::RequestTemplate,
        group_by: Option<GroupBy>,
        dl_id: Option<DataLoaderId>,
+        http_filter: HttpFilter,


This could be optional. If not specified, we should not unnecessarily make a round trip to the JS runtime.

if we dont have/want ant filter then we pass HttpFilter::default(). WE have a check in request Filter logic to call onRequest or execute directly.

src/core/mod.rs

tusharmath · 2024-05-18T13:27:34Z

tests/core/http.rs

+        &self,
+        req: reqwest::Request,
+        _http_filter: &'life0 HttpFilter,
+    ) -> anyhow::Result<Response<Bytes>> {


Too much code here, can we reuse execute?

tusharmath · 2024-05-18T13:28:03Z

tailcall-cloudflare/src/http.rs

+        .await?;
+        tracing::info!("{} {} {}", method, url, res.status.as_u16());
+        Ok(res)
+    }


We should just call execute here.

tusharmath · 2024-05-18T13:28:17Z

tailcall-aws-lambda/src/http.rs

+        let res = Response::from_reqwest(response).await?;
+        tracing::info!("{} {}", req_str, res.status.as_u16());
+        Ok(res)
+    }


just call execute here.

tusharmath · 2024-05-18T13:43:47Z

src/core/blueprint/operators/http.rs

            if !http.group_by.is_empty() && http.method == Method::GET {
                Expression::IO(IO::Http {
                    req_template,
                    group_by: Some(GroupBy::new(http.group_by.clone())),
                    dl_id: None,
+                    http_filter,


Filter should be optional

tusharmath · 2024-05-18T13:45:18Z

src/core/http/mod.rs

+/// User can configure the filter/interceptor
+/// for the http requests.
+pub struct HttpFilter {
+    pub on_request: Option<String>,


I think on_request should not be optional. There is no utility a HttpFilter that doesn't have any on_request.

Co-authored-by: Tushar Mathur <tusharmath@gmail.com>

github-actions · 2024-05-21T06:03:19Z

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

# Conflicts: # src/cli/javascript/request_filter.rs

github-actions · 2024-05-27T20:02:48Z

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

github-actions bot added the type: feature Brand new functionality, features, pages, workflows, endpoints, etc. label May 5, 2024

webbdays force-pushed the 1271 branch 2 times, most recently from f7ab336 to f9669b7 Compare May 10, 2024 03:25

github-actions bot added the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label May 12, 2024

tusharmath marked this pull request as draft May 15, 2024 12:15

github-actions bot removed the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label May 15, 2024

multiple onRequest

0ee2a2e

webbdays force-pushed the 1271 branch from f9669b7 to 0ee2a2e Compare May 16, 2024 04:51

webbdays added 3 commits May 16, 2024 19:49

merge with conflicts resolution

b65fdd2

fix

dee4e26

lint

a0d62a0

github-actions bot added the ci: benchmark Runs benchmarks label May 16, 2024

test

fb195f5

webbdays marked this pull request as ready for review May 17, 2024 03:13

webbdays and others added 3 commits May 18, 2024 09:36

Merge branch 'main' into 1271

91cc51f

move HttpFilter up into Http module

eec2cce

refactor: HyypFilter

8258fd5

tusharmath reviewed May 18, 2024

View reviewed changes

src/core/grpc/data_loader.rs Show resolved Hide resolved

tusharmath reviewed May 18, 2024

View reviewed changes

src/core/graphql/data_loader.rs Show resolved Hide resolved

tusharmath reviewed May 18, 2024

View reviewed changes

src/core/mod.rs Show resolved Hide resolved

tusharmath reviewed May 18, 2024

View reviewed changes

tusharmath removed the ci: benchmark Runs benchmarks label May 18, 2024

Update src/core/mod.rs

50688ca

Co-authored-by: Tushar Mathur <tusharmath@gmail.com>

github-actions bot added the ci: benchmark Runs benchmarks label May 18, 2024

github-actions bot added the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label May 21, 2024

tusharmath added 2 commits May 22, 2024 19:01

remove option from HttpFilter

cf016a4

remove done fixme

40e9987

tusharmath mentioned this pull request May 24, 2024

chore: move worker logic into IO #2017

Open

tusharmath added 2 commits May 26, 2024 00:46

Merge branch 'main' into pr/webbdays/1863

a211c27

# Conflicts: # src/cli/javascript/request_filter.rs

Merge branch 'main' into 1271

539c98f

tusharmath removed the ci: benchmark Runs benchmarks label May 25, 2024

github-actions bot added ci: benchmark Runs benchmarks and removed state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. labels May 25, 2024

github-actions bot added state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. and removed state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. labels May 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: multiple onRequest handlers #1863

feat: multiple onRequest handlers #1863

webbdays commented May 5, 2024

codecov bot commented May 5, 2024 •

edited

github-actions bot commented May 12, 2024

tusharmath commented May 15, 2024

tusharmath commented May 15, 2024

webbdays commented May 16, 2024

github-actions bot commented May 17, 2024 •

edited

tusharmath May 18, 2024 •

edited

webbdays May 18, 2024

tusharmath May 18, 2024

webbdays May 19, 2024

tusharmath May 18, 2024

webbdays May 18, 2024

tusharmath May 18, 2024

webbdays May 18, 2024

tusharmath May 18, 2024

webbdays May 18, 2024

tusharmath May 18, 2024

webbdays May 18, 2024

tusharmath May 18, 2024

webbdays May 18, 2024

tusharmath May 18, 2024

webbdays May 18, 2024

tusharmath May 18, 2024

tusharmath May 18, 2024

github-actions bot commented May 21, 2024

github-actions bot commented May 27, 2024

feat: multiple onRequest handlers #1863

Are you sure you want to change the base?

feat: multiple onRequest handlers #1863

Conversation

webbdays commented May 5, 2024

codecov bot commented May 5, 2024 • edited

Codecov Report

github-actions bot commented May 12, 2024

tusharmath commented May 15, 2024

tusharmath commented May 15, 2024

webbdays commented May 16, 2024

github-actions bot commented May 17, 2024 • edited

Bencher

tusharmath May 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 21, 2024

github-actions bot commented May 27, 2024

codecov bot commented May 5, 2024 •

edited

github-actions bot commented May 17, 2024 •

edited

tusharmath May 18, 2024 •

edited