Split lambda instrumentation into core and events #5326

anuraaga · 2022-02-09T07:18:27Z

Sorry for huge PR couldn't see a way to split up the PR since it's actually splitting up the code.

I had one big misunderstanding about the Java Lambda runtime, that it would include libraries like Jackson for our use. But it turns out unlike some other languages like Python, Java doesn't provide any libraries, presumably since Java provides good classloader isolation techniques.

This PR makes several changes

Splits instrumentation into two parts.
- Target aws-lambda-java-core, which is mostly for supporting RequestStreamHandler. Notably, most lambda functions which use an actual web framework built on top of Lambda (using aws-serverless-java-container or AWS's internal framework) only include this dependency and do not include -events nor should it need to. Core is mostly about instrumenting and does not contain features like serialization.
- Target aws-lambda-java-events, which is mostly for the lambda wrapper of RequestHandler, including support for events. Because Lambda does not know about the user's function type when invoking our wrapper, we must reproduce Lambda's serialization logic when invoking the actual user function.
  - Instrumenter logic only applying to events is only in this module
Include Jackson as a dependency
- lambda-core depends on Jackson core for extracting headers for HTTP propagation. Ideally instrumentation doesn't need external dependencies but this feature is too important to lose in core I think
- lambda-events depends on Jackson databind. In the future, we will replace that with depending on aws-lambda-java-serialization. I've been concerned about the compatibility of our object mapping and finally found that we can reuse Lambda's serialization logic through that logic. I suspect that all of these types are currently incompatible (except SQS which we have a special wrapper for).

anuraaga

Tried to explain some more interesting parts or large diffs in moved files

anuraaga · 2022-02-09T07:19:00Z

...entelemetry/javaagent/instrumentation/awslambdacore/v1_0/AwsLambdaInstrumentationHelper.java

+import io.opentelemetry.instrumentation.awslambdacore.v1_0.internal.AwsLambdaFunctionInstrumenter;
+import io.opentelemetry.instrumentation.awslambdacore.v1_0.internal.AwsLambdaFunctionInstrumenterFactory;
+
+public final class AwsLambdaInstrumentationHelper {


Unchanged except for removing reference to SQS instrumenter

anuraaga · 2022-02-09T07:20:25Z

...try/javaagent/instrumentation/awslambdacore/v1_0/AwsLambdaRequestHandlerInstrumentation.java

+        @Advice.Argument(1) Context context,
+        @Advice.Local("otelInput") AwsLambdaRequest input,
+        @Advice.Local("otelContext") io.opentelemetry.context.Context otelContext,
+        @Advice.Local("otelScope") Scope otelScope) {


Unchanged except removing references to SQS instrumenter

anuraaga · 2022-02-09T07:20:45Z

...rary/src/main/java/io/opentelemetry/instrumentation/awslambdacore/v1_0/LambdaParameters.java

+
+final class LambdaParameters {
+
+  static <T> Object[] toArray(Method targetMethod, T input, Context context) {


Removed mapper for core version of this util

anuraaga · 2022-02-09T07:21:36Z

...src/main/java/io/opentelemetry/instrumentation/awslambdacore/v1_0/TracingRequestHandler.java

+   * Returns the HTTP headers for the request extracted from the request handler's input. By
+   * default, returns empty headers.
+   */
+  protected Map<String, String> extractHttpHeaders(I input) {


This is the most invasive change, since a handler that uses ApiGatewayProxyEvent would need to have its HTTP headers extracted. That being said, headers could be represented by input in other unknown ways so giving a hook into that could be considered generally useful (with limited practical use case).

anuraaga · 2022-02-09T07:21:57Z

...ava/io/opentelemetry/instrumentation/awslambdacore/v1_0/internal/ApiGatewayProxyRequest.java

    if (noHttpPropagationNeeded()) {
      return new NoopRequest(source);
    }

    if (source.markSupported()) {
      return new MarkableApiGatewayProxyRequest(source);
    }
-    // fallback
-    return new CopiedApiGatewayProxyRequest(source);
+    // It is known that the Lambda runtime passes ByteArrayInputStream's to functions, so gracefully


I believe this is reasonable. Notably, if we were actually hitting the fallback codepath, it would often be failing with ClassNotFoundException since commons-io is not referenced at all by lambda libraries and would only have to be available by luck

anuraaga · 2022-02-09T07:24:35Z

...telemetry/javaagent/instrumentation/awslambdaevents/v2_2/AwsLambdaInstrumentationHelper.java

+import io.opentelemetry.instrumentation.awslambdaevents.v2_2.internal.AwsLambdaEventsInstrumenterFactory;
+import io.opentelemetry.instrumentation.awslambdaevents.v2_2.internal.AwsLambdaSqsInstrumenterFactory;
+
+public final class AwsLambdaInstrumentationHelper {


Same as helper in current HEAD code

anuraaga · 2022-02-09T07:24:48Z

...telemetry/javaagent/instrumentation/awslambdaevents/v2_2/AwsLambdaInstrumentationModule.java

+  }
+
+  @Override
+  public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {


Added this matcher, verified by testInstrumentation in other project

anuraaga · 2022-02-09T07:25:26Z

...y/javaagent/instrumentation/awslambdaevents/v2_2/AwsLambdaRequestHandlerInstrumentation.java

@@ -63,18 +61,24 @@ public static void onEnter(
        @Advice.Local("otelMessageContext") io.opentelemetry.context.Context messageContext,
        @Advice.Local("otelMessageScope") Scope messageScope) {


This class is same as current instrumentation in HEAD

It's probably not enough to figure out a way to share logic in the advice classes between the two javaagent instrumentations at this time

anuraaga · 2022-02-09T07:26:13Z

...in/java/io/opentelemetry/instrumentation/awslambdaevents/v2_2/TracingRequestWrapperBase.java

+    super(
+        openTelemetrySdk,
+        WrapperConfiguration.flushTimeout(),
+        AwsLambdaEventsInstrumenterFactory.createInstrumenter(openTelemetrySdk));


Key change - the instrumenter factory in this module is aware of event objects

anuraaga · 2022-02-09T07:26:41Z

...emetry/instrumentation/awslambdaevents/v2_2/internal/AwsLambdaEventsInstrumenterFactory.java


 import com.amazonaws.services.lambda.runtime.events.APIGatewayProxyRequestEvent;
 import io.opentelemetry.api.OpenTelemetry;
 import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
 import io.opentelemetry.instrumentation.api.instrumenter.SpanKindExtractor;
-import io.opentelemetry.instrumentation.awslambda.v1_0.AwsLambdaRequest;
+import io.opentelemetry.instrumentation.awslambdacore.v1_0.AwsLambdaRequest;
+import io.opentelemetry.instrumentation.awslambdacore.v1_0.internal.AwsLambdaFunctionAttributesExtractor;


We use the core instrumentation logic, augmenting with event-specific logic

…-instrumentation into split-lambda

trask · 2022-02-10T03:58:29Z

instrumentation/aws-lambda/aws-lambda-core-1.0/library/build.gradle.kts

+
+  implementation("io.opentelemetry:opentelemetry-extension-aws")
+
+  implementation("com.fasterxml.jackson.core:jackson-core")


I think worth adding comment here (similar to your PR comment) since this is unusual

trask · 2022-02-10T04:06:04Z

...emetry/instrumentation/awslambdacore/v1_0/internal/AwsLambdaFunctionAttributesExtractor.java

+/**
+ * This class is internal and is hence not for public use. Its APIs are unstable and can change at
+ * any time.
+ */


trask · 2022-02-10T04:11:51Z

instrumentation/aws-lambda/aws-lambda-events-2.2/library/build.gradle.kts

-  compileOnly("commons-io:commons-io:2.2")
-  compileOnly("org.slf4j:slf4j-api")
-
+  implementation("com.fasterxml.jackson.core:jackson-databind")


and copy in comment here from your PR comment would be nice I think

trask · 2022-02-10T04:16:30Z

instrumentation/aws-lambda/aws-lambda-events-2.2/javaagent/build.gradle.kts

+  implementation(project(":instrumentation:aws-lambda:aws-lambda-core-1.0:library"))
+
+  implementation(project(":instrumentation:aws-lambda:aws-lambda-events-2.2:library")) {
+    // Only needed by wrappers, not the javaagent. Muzzle will catch if we accidentally change this.


* Split out lambda core instrumentation * More * Remove request wrapper from core * Split events * Dedupe * More * More renames * Finish * Clean * README * Rename more * Finish * Fix README * Fix README * Fix * Comments

anuraaga added 15 commits February 9, 2022 04:07

Split out lambda core instrumentation

a7fed5e

More

c794038

Remove request wrapper from core

12b7ebd

Split events

60f6273

Dedupe

3d30c86

More

1ee27ce

More renames

3bc9432

Finish

e232ed4

Clean

c32f36c

README

f1410ac

Rename more

739e90c

Finish

29c4149

Fix README

a8af13a

Fix README

ceef310

Fix

d279140

anuraaga requested a review from a team as a code owner February 9, 2022 07:18

anuraaga mentioned this pull request Feb 9, 2022

Set jackson.databind as 'implementation' dep for Lambda Instrumentation #5284

Closed

anuraaga commented Feb 9, 2022

View reviewed changes

Merge branch 'split-lambda' of github.com:anuraaga/opentelemetry-java…

c3a2bb3

…-instrumentation into split-lambda

mateuszrzeszutek requested a review from a user February 9, 2022 09:43

anuraaga added this to the 1.11.0 milestone Feb 10, 2022

trask approved these changes Feb 10, 2022

View reviewed changes

Comments

48fa69a

trask merged commit f236b2d into open-telemetry:main Feb 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split lambda instrumentation into core and events #5326

Split lambda instrumentation into core and events #5326

anuraaga commented Feb 9, 2022 •

edited

Loading

anuraaga left a comment

anuraaga Feb 9, 2022

anuraaga Feb 9, 2022

anuraaga Feb 9, 2022

anuraaga Feb 9, 2022

anuraaga Feb 9, 2022

anuraaga Feb 9, 2022

anuraaga Feb 9, 2022

anuraaga Feb 9, 2022

anuraaga Feb 9, 2022

anuraaga Feb 9, 2022

trask Feb 10, 2022

trask Feb 10, 2022

trask Feb 10, 2022

trask Feb 10, 2022


		final class LambdaParameters {

		static <T> Object[] toArray(Method targetMethod, T input, Context context) {

		@@ -63,18 +61,24 @@ public static void onEnter(
		@Advice.Local("otelMessageContext") io.opentelemetry.context.Context messageContext,
		@Advice.Local("otelMessageScope") Scope messageScope) {


		implementation("io.opentelemetry:opentelemetry-extension-aws")

		implementation("com.fasterxml.jackson.core:jackson-core")

Split lambda instrumentation into core and events #5326

Split lambda instrumentation into core and events #5326

Conversation

anuraaga commented Feb 9, 2022 • edited Loading

anuraaga 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

anuraaga commented Feb 9, 2022 •

edited

Loading