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

DRIVERS-2830 - FaaS detection logic mistakenly identifies EKS as AWS Lambda #1673

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

NoahStapp
Copy link
Contributor

@NoahStapp NoahStapp commented Oct 9, 2024

DRIVERS-2830 Summary of changes:

  • If both a FaaS provider and a container are present, both metadatas must be recorded.

Python implementation: mongodb/mongo-python-driver#1908.

@@ -327,6 +327,8 @@ Depending on which `client.env.name` has been selected, other FaaS fields in `cl
Missing variables or variables with values not matching the expected type MUST cause the corresponding `client.env`
field to be omitted and MUST NOT cause a user-visible error.

If any fields of `client.env.container` are populated, all FaaS values MUST be entirely omitted.
Copy link
Member

Choose a reason for hiding this comment

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

What about cases where a container is deployed on AWS Lambda? https://docs.aws.amazon.com/lambda/latest/dg/images-create.html

Wouldn't we want to include both in that case?

IIRC the issue that motivated this drivers ticket was a customer was not using Lambda but lambda still showed up in the client metadata. It looks like they were using pymongo 4.6.0. We added container detection in pymongo 4.7: PYTHON-3837.

@NoahStapp NoahStapp marked this pull request as ready for review October 11, 2024 14:52
@NoahStapp NoahStapp requested a review from a team as a code owner October 11, 2024 14:52
@NoahStapp NoahStapp requested review from dariakp and removed request for a team October 11, 2024 14:52
@NoahStapp
Copy link
Contributor Author

@@ -511,6 +511,16 @@ the following sets of environment variables:
| -------------------- | ----- |
| `AWS_EXECUTION_ENV` | `EC2` |

9. Valid container and FaaS provider. This test MUST verify that both the container metadata and the AWS Lambda metadata
Copy link
Member

Choose a reason for hiding this comment

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

We should include a note about this behavior in the client.env description above as well.

@blink1073
Copy link
Member

Perhaps update that link to https://en.wikipedia.org/wiki/Balls_into_bins_problem?

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

Successfully merging this pull request may close these issues.

3 participants