Implement AWS extension as an actor #1277

sl0thentr0py · 2022-05-19T14:13:47Z

Implement an actor AwsExtension that starts in an Arbiter conditionally on the --aws-runtime-api cli option
Also add --aws-upstream-dsn that sets the UpstreamDescriptor from the explicitly passed in DSN
Uses a reqwest client with 0 timeout in a separate tokio::runtime with 1 worker thread internally to register and poll for next events continuously
- note there is no sleep here because the requests to next_event will block till next invocation
- the container also might get frozen in case of inactivity
A mock flask server for testing the lifecycle
Also note that we don't do anything really useful with the responses from AWS but the protocol is implemented fully and we could use some of this information in the future to add to the envelope/transaction

https://docs.aws.amazon.com/lambda/latest/dg/runtimes-extensions-api.html#runtimes-extensions-registration-api

antonpirker

I can not review the rust code so my review is more from a UX/product angle.

relay-aws-extension/src/aws_extension.rs

jan-auer

Initial review with merely technicalities. Review of the actor follows momentarily.

jan-auer · 2022-05-20T08:29:40Z

CHANGELOG.md

@@ -7,6 +7,7 @@
 **Features**:

 - Session metrics extraction: Count crashed+abnormal towards errored_preaggr. ([#1274](https://github.com/getsentry/relay/pull/1274))
+- Implement AWS extension as an actor. ([#1277](https://github.com/getsentry/relay/pull/1277))


Let's make this a slightly more descriptive changelog entry around what the feature enables.

relay-aws-extension/Cargo.toml

jan-auer · 2022-05-20T08:35:56Z

relay-aws-extension/mock/run.sh

+pip install -r requirements.txt
+
+# Start mock API server
+FLASK_APP="mock-aws-lambda-extensions-api" flask run


Since we're already running flask apps in integration tests (see tests/integration/*), what if you move this flask app as fixture there, and then create a new integration test file for this?

relay-aws-extension/src/aws_extension.rs

relay-server/Cargo.toml

relay-aws-extension/src/lib.rs

jan-auer · 2022-05-20T08:41:06Z

relay-server/src/service.rs

@@ -143,6 +145,12 @@ impl ServiceState {
        let outcome_aggregator = OutcomeAggregator::new(&config, outcome_producer.recipient());
        registry.set(outcome_aggregator.start());

+        if let Ok(aws_runtime_api) = env::var("AWS_LAMBDA_RUNTIME_API") {


nit: Move this into a (documented) function in the relay-aws-extension crate.

For as long as we're just experimenting (if not for longer), I would strongly prefer a config flag for this in addition to the envvar. There is no telling what would happen if an existing, working customer relay is updated to include this code.

added a --aws-api-url=<URL> cli arg

relay-aws-extension/src/aws_extension.rs

jan-auer · 2022-05-20T09:00:27Z

relay-aws-extension/src/aws_extension.rs

+impl AwsExtension {
+    /// Creates a new `AwsExtension` instance.
+    pub fn new(aws_runtime_api: String) -> Result<Self, AwsExtensionError> {
+        let base_url = format!("http://{}/2020-01-01/extension", aws_runtime_api);


Which additional validation do we need to put here to ensure this doesn't become an attack vector?

relay-aws-extension/src/aws_extension.rs

jan-auer · 2022-05-20T09:05:53Z

relay-aws-extension/src/aws_extension.rs

+            .post(&url)
+            .header(EXTENSION_NAME_HEADER, EXTENSION_NAME)
+            .json(&map)
+            .send()


You're using the blocking client here, which is not ideal since it blocks the arbiter this actor is running in. You have two options:

Use the non-blocking client in a tokio 1 runtime. We will soon clean this all up to make it easier, but in the meanwhile, you'll want to implement it like here (which means, you can use .await to chain several calls):

relay/relay-server/src/actors/upstream.rs

Lines 581 to 587 in ef952cb

self.reqwest_runtime.spawn(async move {

let res = client

.execute(client_request.0)

.await

.map_err(UpstreamRequestError::SendFailed);

tx.send(res)

});

Document that this actor must run in its completely own thread and place a big TODO on top of this to clean it up as soon as we've migrated to tokio 1. However, that also means you cannot receive messages such as the shutdown while you're waiting for a response, so I'm not sure if that's even a feasible option.

Given the similarity to UpstreamRelay, I'd prefer option 1.

I would also prefer option 1. Note that with option 1, the entire actor can be one big async block containing a loop {} (no need for context.notify)

It may be useful to keep the outermost loop in a notify cycle so we can break it more easily from the outside, although that doesn't seem required for AWS environments.

this is done now @jan-auer @untitaker, if you guys can take a look that the approach is ok?

relay-aws-extension/src/aws_extension.rs

untitaker · 2022-05-20T09:28:49Z

relay-aws-extension/src/aws_extension.rs

+            }
+            _ => {
+                relay_log::debug!("Next event request failed");
+                context.notify(NextEvent);


This will keep hammering the API endpoint if it is not available. Is that desired behavior? If so I would ask you to hide this actor behind a config option, today it turns on based on whether some envvar is present.

I guess I need to distinguish between 404s and other failures here.

untitaker · 2022-05-20T09:30:15Z

relay-aws-extension/src/aws_extension.rs

+            .post(&url)
+            .header(EXTENSION_NAME_HEADER, EXTENSION_NAME)
+            .json(&map)
+            .send()


I would also prefer option 1. Note that with option 1, the entire actor can be one big async block containing a loop {} (no need for context.notify)

Co-authored-by: Jan Michael Auer <[email protected]>

untitaker

I think this is fine for now. Let's just make sure we can't break existing customer relays by accident.

untitaker · 2022-05-25T12:52:45Z

relay-server/src/service.rs

@@ -143,6 +145,12 @@ impl ServiceState {
        let outcome_aggregator = OutcomeAggregator::new(&config, outcome_producer.recipient());
        registry.set(outcome_aggregator.start());

+        if let Ok(aws_runtime_api) = env::var("AWS_LAMBDA_RUNTIME_API") {


For as long as we're just experimenting (if not for longer), I would strongly prefer a config flag for this in addition to the envvar. There is no telling what would happen if an existing, working customer relay is updated to include this code.

untitaker · 2022-05-25T12:53:03Z

CHANGELOG.md

@@ -7,6 +7,7 @@
 **Features**:

 - Session metrics extraction: Count crashed+abnormal towards errored_preaggr. ([#1274](https://github.com/getsentry/relay/pull/1274))
+- Implement AWS extension as an actor. ([#1277](https://github.com/getsentry/relay/pull/1277))


relay/src/cliapp.rs

… ARM based CPUs.

jan-auer

Thanks for the effort on this, let's get this merged.

* master: ref(metrics): Stop logging relative bucket size (#1302) fix(metrics): Rename misnamed aggregator option (#1298) fix(server): Avoid a panic in the Sentry middleware (#1301) build: Update dependencies with known vulnerabilities (#1294) fix(metrics): Stop logging statsd metric per project key (#1295) feat(metrics): Limits on bucketing cost in aggregator [INGEST-1132] (#1287) fix(metrics): Track memory footprint more accurately (#1288) build(deps): Bump dependencies (#1293) feat(aws): Add relay-aws-extension crate which implements AWS extension as an actor (#1277) fix(meta): Update codeowners for the release actions (#1286) feat(metrics): Track memory footprint of metrics buckets (#1284)

sl0thentr0py added 9 commits May 19, 2022 13:30

New relay-aws-extension crate with actor, start in Arbiter

5e9988c

Implement main loop with NextEvent message

5daed97

Implement register flow

107612d

Implement RegisterResponse

634716d

Implement NextEventResponse

89bab1c

Kill whole system on SHUTDOWN

3dc8051

Remove unused const

9dabfa2

Cleanup unwraps

a02680c

Docs

d53092f

sl0thentr0py requested review from a team and antonpirker May 19, 2022 14:13

Changelog

d2148cc

antonpirker reviewed May 19, 2022

View reviewed changes

jan-auer reviewed May 20, 2022

View reviewed changes

jan-auer requested changes May 20, 2022

View reviewed changes

untitaker reviewed May 20, 2022

View reviewed changes

sl0thentr0py and others added 9 commits May 20, 2022 12:37

Update relay-aws-extension/src/aws_extension.rs

23426cb

Co-authored-by: Jan Michael Auer <[email protected]>

Make structs pub and doc

82a80a0

Misc code style

613d66c

Don't set registry and don't implement Default/Supervised/SystemService

d4e5b55

Add entry in main relay docs

3d62d2b

Add tokio runtime for reqwest

51847a2

Missing log entry

e94f476

Remove make_request

4d00fbf

Export Signal, SignalType from relay-system

8352fdb

untitaker requested changes May 25, 2022

View reviewed changes

sl0thentr0py added 4 commits May 25, 2022 15:42

Better docs

3169b49

Merge remote-tracking branch 'origin/master' into neel/aws-ext-actor

fe0d699

Move from env variable to separate --aws-api-url config

c827d74

Better changelog

9d59928

jan-auer and others added 3 commits May 30, 2022 17:26

ref: Rename CLI arg and update docs

5e638c3

Fix path

98a1d23

Move aws ext mock to integration test / fixture

0318904

andriisoldatenko reviewed Jun 1, 2022

View reviewed changes

relay/src/cliapp.rs Outdated Show resolved Hide resolved

sl0thentr0py added 3 commits June 1, 2022 13:33

Add to CODEOWNERS

792caab

Add aws-upstream-dsn cli arg for run

07508bd

Fix typo CODEOWNERS

03268b4

sl0thentr0py force-pushed the neel/aws-ext-actor branch from 41a606d to 03268b4 Compare June 1, 2022 12:58

sl0thentr0py and others added 3 commits June 1, 2022 16:12

Merge remote-tracking branch 'origin/master' into neel/aws-ext-actor

c149349

Added readme with introductions on how to compile Lambda Extension on…

260ab99

… ARM based CPUs.

ref(cli): Make --upstream-dsn a generic feature

efb1178

jan-auer approved these changes Jun 3, 2022

View reviewed changes

Merge branch 'master' into neel/aws-ext-actor

49542b9

untitaker approved these changes Jun 7, 2022

View reviewed changes

Fix CHANGELOG

fc00aa1

sl0thentr0py enabled auto-merge (squash) June 7, 2022 10:38

sl0thentr0py merged commit 564b7e5 into master Jun 7, 2022

sl0thentr0py deleted the neel/aws-ext-actor branch June 7, 2022 10:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement AWS extension as an actor #1277

Implement AWS extension as an actor #1277

sl0thentr0py commented May 19, 2022 •

edited

Loading

antonpirker left a comment

jan-auer left a comment

jan-auer May 20, 2022

untitaker May 25, 2022

jan-auer May 20, 2022

jan-auer May 20, 2022

untitaker May 25, 2022

sl0thentr0py May 25, 2022

jan-auer May 20, 2022

jan-auer May 20, 2022

untitaker May 20, 2022

jan-auer May 20, 2022

sl0thentr0py May 25, 2022

untitaker May 20, 2022

sl0thentr0py May 20, 2022

untitaker May 20, 2022

untitaker left a comment

untitaker May 25, 2022

untitaker May 25, 2022

jan-auer left a comment

	self.reqwest_runtime.spawn(async move {
	let res = client
	.execute(client_request.0)
	.await
	.map_err(UpstreamRequestError::SendFailed);
	tx.send(res)
	});

Implement AWS extension as an actor #1277

Implement AWS extension as an actor #1277

Conversation

sl0thentr0py commented May 19, 2022 • edited Loading

antonpirker left a comment

Choose a reason for hiding this comment

jan-auer left a 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

untitaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-auer left a comment

Choose a reason for hiding this comment

sl0thentr0py commented May 19, 2022 •

edited

Loading