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

implements context propagation for lambda invoke + tests #458

Merged
merged 1 commit into from May 14, 2021
Merged

implements context propagation for lambda invoke + tests #458

merged 1 commit into from May 14, 2021

Conversation

ghost
Copy link

@ghost ghost commented Apr 16, 2021

Description

Implements context propagation for lambda invoke (botocore) which needs to have the headers as a part of the payload.
Adds tests.
Bumps moto to ~2.0 - previous version had issues preventing lambda tests.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Automated test with moto

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@ghost ghost requested review from a team, codeboten and ocelotl and removed request for a team April 16, 2021 20:11
@ghost
Copy link
Author

ghost commented Apr 19, 2021

For Py38, the instrumentation tests fail with:

ImportError while importing test module '/home/runner/work/opentelemetry-python-contrib/opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-botocore/tests/test_botocore_instrumentation.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/opt/hostedtoolcache/Python/3.8.9/x64/lib/python3.8/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
test_botocore_instrumentation.py:61: in <module>
    class TestBotocoreInstrumentor(TestBase):
test_botocore_instrumentation.py:332: in TestBotocoreInstrumentor
    def test_lambda_client(self):
../../../.tox/py38-test-instrumentation-botocore/lib/python3.8/site-packages/moto/__init__.py:8: in f
    module = importlib.import_module(module_name, "moto")
/opt/hostedtoolcache/Python/3.8.9/x64/lib/python3.8/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
../../../.tox/py38-test-instrumentation-botocore/lib/python3.8/site-packages/moto/awslambda/__init__.py:2: in <module>
    from .models import lambda_backends
../../../.tox/py38-test-instrumentation-botocore/lib/python3.8/site-packages/moto/awslambda/models.py:10: in <module>
    import docker
E   ModuleNotFoundError: No module named 'docker'
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!

Any hints regarding how to fix that?

@ghost
Copy link
Author

ghost commented Apr 28, 2021

@codeboten @ocelotl @owais please have a look at the PR :)

@ghost
Copy link
Author

ghost commented May 5, 2021

@owais I finally run some microbenchmarking of the code and results are... interesting. They are heavily dependant on the input provided, as in case of "headers" property included in the payload the algo checks if it's actually a first-level children by counting curly braces.

Test setup:

  • 100000 iterations
  • payload containing 100 entries, flat structure
  • run with headers property near beginning, near end and without the property at all

Results (in ms):

JSON-based:  4.218646998
String-based no headers:  0.5552989840000002
String-based with headers:  19.781614840000003
String-based with headers at the beginning:  1.2955868240000008

In general without headers property set or with property near the beginning improved (string-based) algorithm is much faster (3,5x - 8x). However for bigger payloads with headers near the end the situation changes - JSON based implementation becomes 5x faster.

I believe that for most cases we should not expect "headers" property to appear in the lambda invoke payload as supplied by the customer. So in the end the decision which way to include should be based on our readiness to support slightly more complex code for a really small performance gain (100000 iterations - 4 ms vs 0,5 ms).

@ghost
Copy link
Author

ghost commented May 5, 2021

@codeboten @ocelotl please have a look, it's been in review for last 16 days :)

@owais
Copy link
Contributor

owais commented May 6, 2021

In general without headers property set or with property near the beginning improved (string-based) algorithm is much faster (3,5x - 8x). However for bigger payloads with headers near the end the situation changes - JSON based implementation becomes 5x faster.
I believe that for most cases we should not expect "headers" property to appear in the lambda invoke payload as supplied by the customer. So in the end the decision which way to include should be based on our readiness to support slightly more complex code for a really small performance gain (100000 iterations - 4 ms vs 0,5 ms).

Given the small gains in only a subset of cases, I think we probably don't want the additional complexity of custom parsing. I'm inclined towards going with stock JSON module but happy to hear from others.

@codeboten codeboten self-assigned this May 6, 2021
@codeboten
Copy link
Contributor

@kubawach if you could take a look at the lint failure, we can get this PR merged as soon as it passes CI

@ghost
Copy link
Author

ghost commented May 12, 2021

@codeboten fixed linting and rebased to current main, resolving some conflicts by the way

@ghost
Copy link
Author

ghost commented May 13, 2021

Seems that all of the conflicts have been resolved, please merge as convenient :)

@codeboten
Copy link
Contributor

@kubawach looks like there's still some issues that need resolving w/ lint and tests failing.

@ghost
Copy link
Author

ghost commented May 14, 2021

@codeboten sorry for premature alarm. Now it really looks as ready to merge (I even took screenshot to be 101% sure ;) )
image

@codeboten
Copy link
Contributor

Thanks @kubawach!

@codeboten codeboten merged commit c8ec25a into open-telemetry:main May 14, 2021
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