Skip to content

Feature: Decompose submit notification for s3 to be able to patch it in extension #11692

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
Merged
merged 1 commit into from
Oct 15, 2024
Merged

Feature: Decompose submit notification for s3 to be able to patch it in extension #11692

merged 1 commit into from
Oct 15, 2024

Conversation

maxhoheiser
Copy link
Member

Motivation

We need to be able to submit the threading.local context to the child thread where the notification is handled so we can pass trace context. We can do this by patching the elf.executor.submit method and only calling the patched method, for this we need to decompose the send_notifications method.

@maxhoheiser maxhoheiser added the aws:s3 Amazon Simple Storage Service label Oct 15, 2024
@maxhoheiser maxhoheiser self-assigned this Oct 15, 2024
@maxhoheiser maxhoheiser requested a review from bentsku as a code owner October 15, 2024 11:49
@maxhoheiser maxhoheiser added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Oct 15, 2024
@github-actions GitHub Actions
Copy link

S3 Image Test Results (AMD64 / ARM64)

  2 files    2 suites   3m 42s ⏱️
423 tests 369 ✅  54 💤 0 ❌
846 runs  738 ✅ 108 💤 0 ❌

Results for commit a69f653.

bentsku
bentsku reviewed Oct 15, 2024
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

I would like to have a bit more context regarding how the patching will be done, but the change looks alright! If it's possible to \cc me in the related PR, that would be great 😄

@github-actions GitHub Actions
Copy link

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   55m 6s ⏱️ - 49m 15s
1 555 tests  - 1 940  1 379 ✅  - 1 703  176 💤  - 237  0 ❌ ±0 
1 557 runs   - 1 940  1 379 ✅  - 1 703  178 💤  - 237  0 ❌ ±0 

Results for commit a69f653. ± Comparison against base commit 5e8d3a1.

This pull request removes 1940 tests. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

bentsku
bentsku approved these changes Oct 15, 2024
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM!

@maxhoheiser maxhoheiser merged commit ff397dc into master Oct 15, 2024
44 of 46 checks passed
@maxhoheiser maxhoheiser deleted the feature/eventstudio/s3-patch-submit-notification branch October 15, 2024 13:45
macnev2013 pushed a commit that referenced this pull request Oct 22, 2024
@maxhoheiser @macnev2013
Feature: Decompose submit notification for s3 to be able to patch it …
43bc5a6 
…in extension (#11692)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bentsku bentsku bentsku approved these changes

Assignees

@maxhoheiser maxhoheiser

Labels
aws:s3 Amazon Simple Storage Service semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@maxhoheiser @bentsku