opentelemetry-lambda icon indicating copy to clipboard operation
opentelemetry-lambda copied to clipboard

remove unnecessary call to AwsLambdaInstrumentor().instrument() in wrapper script

Open herin049 opened this issue 1 month ago • 4 comments

Fixes: #2068

herin049 avatar Dec 11 '25 04:12 herin049

While removing explicit AwsLambdaInstrumentor().instrument() seems OK to me, I am not fully sure on

  • might there be a side effect? did you try the changes here with a custom layer?
  • why it added before? was AwsLambdaInstrumentor not part of default list of auto instrumented packages before?

serkan-ozal avatar Dec 13 '25 17:12 serkan-ozal

While removing explicit AwsLambdaInstrumentor().instrument() seems OK to me, I am not fully sure on

* might there be a side effect? did you try the changes here with a custom layer?

* why it added before? was `AwsLambdaInstrumentor` not part of default list of auto instrumented packages before?

@serkan-ozal Looks like the reasoning was given here in a comment which I accidentally missed:

Usage
-----
Patch the reserved `_HANDLER` Lambda environment variable to point to this
file's `otel_wrapper.lambda_handler` property. Do this having saved the original
`_HANDLER` in the `ORIG_HANDLER` environment variable. Doing this makes it so
that **on import of this file, the handler is instrumented**.

Instrumenting any earlier will cause the instrumentation to be lost because the
AWS Service uses `imp.load_module` to import the handler which RELOADS the
module. This is why AwsLambdaInstrumentor cannot be instrumented with the
`opentelemetry-instrument` script.

Looks like this was added by @NathanielRN

However, I don't think the reasoning here is quite correct. Even IF lambda does reload the module being pointed to by _HANDLER, this will not matter because the original lambda handler pointed to by ORIG_HANDLER lives in a different module and thus, will NOT be reloaded if the lambda runtime reloads the module pointed to by _HANLDER (which is the otel_wrapper module)

(see https://docs.python.org/3/library/importlib.html#importlib.reload)

Even the original approach doesn't accomplish the goal of instrumenting the lambda handler inside the wrapper script because the AWS Lambda instrumentation library is not excluded from being used by auto instrumentation. As a result, the lambda instrumentation library is called before the wrapper script is run and any subsequent attempts to instrument will be a no-op due to the short-circuiting logic inside the Python instrumentation library:

    def instrument(self, **kwargs: Any):
        """Instrument the library

        This method will be called without any optional arguments by the
        ``opentelemetry-instrument`` command.

        This means that calling this method directly without passing any
        optional values should do the very same thing that the
        ``opentelemetry-instrument`` command does.
        """

        if self._is_instrumented_by_opentelemetry:
            _LOG.warning("Attempting to instrument while already instrumented")
            return None

I've tested these changes by building the auto instrumentation layer exactly how it is in this PR with Python 3.14, if you'd like, I can verify that this works on the older lambda runtimes. This also makes me think that we should at some point add an integration test suite that runs in AWS against all supported lambda runtimes - at least before releasing new layer versions. Let me know what you think, we can discuss details further in one of the FaaS SIG meetings. This is something I would be very happy to lead development on.

herin049 avatar Dec 13 '25 18:12 herin049

@herin049 I would test with older (but currently available) Python Lambda runtimes too. AWS Lambda Python runtime clients (which includes bootstrap.py) might have different implementations (we have faced some cases in Nodejs) and AwsLambdaInstrumentor().instrument() call might have been added for some weird trick/case in one of the older Lambda Python runtimes.

serkan-ozal avatar Dec 13 '25 20:12 serkan-ozal

@serkan-ozal I was able to test and manually verify that the changes work with Python 3.9, 3.11, 3.12, 3.13 and 3.14.

herin049 avatar Dec 17 '25 16:12 herin049

This also makes me think that we should at some point add an integration test suite that runs in AWS against all supported lambda runtimes - at least before releasing new layer versions.

Talked about this in the SIG meeting with @tylerbenson today as well. I have an issue open here to track this effort as well: https://github.com/open-telemetry/opentelemetry-lambda/issues/2079

wpessers avatar Dec 18 '25 21:12 wpessers