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.

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

AWS lambda - improvements in custom type handling in wrappers, SQS ev… #4254

Merged
merged 5 commits into from Oct 8, 2021
Merged

AWS lambda - improvements in custom type handling in wrappers, SQS ev… #4254

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
016cc48
AWS lambda - improvements in custom type handling in wrappers, SQS ev…
kuba-wu Sep 30, 2021
f2a79f2
post-review changes
kuba-wu Oct 4, 2021
eabc0d0
Update instrumentation/aws-lambda-1.0/library/src/main/java/io/opente…
kuba-wu Oct 5, 2021
1267d31
Update instrumentation/aws-lambda-1.0/library/src/main/java/io/opente…
kuba-wu Oct 5, 2021
a1a80ee
post-review changes
kuba-wu Oct 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions ...brary/src/main/java/io/opentelemetry/instrumentation/awslambda/v1_0/LambdaParameters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.awslambda.v1_0;

import com.amazonaws.services.lambda.runtime.Context;
import java.lang.reflect.Method;
import java.util.function.BiFunction;

class LambdaParameters {
This conversation was marked as resolved.
Show resolved Hide resolved

static <T> Object[] toArray(
Method targetMethod, T input, Context context, BiFunction<T, Class, Object> mapper) {
Class<?>[] parameterTypes = targetMethod.getParameterTypes();
Object[] parameters = new Object[parameterTypes.length];
for (int i = 0; i < parameterTypes.length; i++) {
Class clazz = parameterTypes[i];
boolean isContext = clazz.equals(Context.class);
if (isContext) {
parameters[i] = context;
} else if (i == 0) {
parameters[0] = (clazz.isInstance(input) ? input : mapper.apply(input, clazz));
}
}
return parameters;
}
}
This conversation was marked as resolved.
Show resolved Hide resolved
43 changes: 39 additions & 4 deletions ...java/io/opentelemetry/instrumentation/awslambda/v1_0/TracingRequestApiGatewayWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,58 @@

package io.opentelemetry.instrumentation.awslambda.v1_0;

import com.amazonaws.services.lambda.runtime.Context;
import com.amazonaws.services.lambda.runtime.events.APIGatewayProxyRequestEvent;
import com.amazonaws.services.lambda.runtime.events.APIGatewayProxyResponseEvent;
import com.fasterxml.jackson.core.JsonProcessingException;
import io.opentelemetry.sdk.OpenTelemetrySdk;
import java.util.function.BiFunction;

/**
* Wrapper for {@link TracingRequestHandler}. Allows for wrapping a lambda proxied through API
* Gateway, enabling single span tracing and HTTP context propagation.
*/
public class TracingRequestApiGatewayWrapper
extends TracingRequestWrapperBase<APIGatewayProxyRequestEvent, APIGatewayProxyResponseEvent> {
extends TracingRequestWrapperBase<APIGatewayProxyRequestEvent, Object> {

public TracingRequestApiGatewayWrapper() {
super();
super(TracingRequestApiGatewayWrapper::map);
}

// Visible for testing
TracingRequestApiGatewayWrapper(OpenTelemetrySdk openTelemetrySdk, WrappedLambda wrappedLambda) {
super(openTelemetrySdk, wrappedLambda);
TracingRequestApiGatewayWrapper(
OpenTelemetrySdk openTelemetrySdk,
WrappedLambda wrappedLambda,
BiFunction<APIGatewayProxyRequestEvent, Class, Object> mapper) {
super(openTelemetrySdk, wrappedLambda, mapper);
}

// Visible for testing
static Object map(APIGatewayProxyRequestEvent event, Class clazz) {
try {
return OBJECT_MAPPER.readValue(event.getBody(), clazz);
} catch (JsonProcessingException e) {
throw new IllegalStateException(
"Could not map API Gateway event body to requested parameter type: " + clazz, e);
}
}

@Override
protected APIGatewayProxyResponseEvent doHandleRequest(
APIGatewayProxyRequestEvent input, Context context) {
Object result = super.doHandleRequest(input, context);
APIGatewayProxyResponseEvent event = null;
// map to response event if needed
if (result instanceof APIGatewayProxyResponseEvent) {
event = (APIGatewayProxyResponseEvent) result;
} else {
try {
event = new APIGatewayProxyResponseEvent();
event.setBody(OBJECT_MAPPER.writeValueAsString(result));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know that our OBJECT_MAPPER is configured the same as Lambda?

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately no guarantees here. Haven't seen an official spec by AWS guys regarding how the JSON is mapped in their code.

} catch (JsonProcessingException e) {
throw new IllegalStateException("Could not serialize return value.", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

These exceptions seem worrisome, are we sure we won't break the app?

Copy link
Author

Choose a reason for hiding this comment

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

The silver lining here is that if the wrapper is unable to map to a custom type, the user can still use the "APIGatewayProxyRequestEvent" and avoid mapping on the wrapper side entirely. So in the worst case it will work exactly the same way it does right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it still means we have the possibility of breaking the lambda just by the presence of a lambda layer such as the opentelemetry-lambda ones. I don't think it's good to have the possibility of adding crashing.

That being said I guess I can't find any alternative unfortunately. It's too bad the Context object doesn't provide information about HTTP requests...

Is it at least beneficial to split into one where the target handler is proxy that doesn't have any mapping and this one that does for custom return type?

Copy link
Author

Choose a reason for hiding this comment

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

It's implemented (kind of) this way - mapping ins only done if the return type of the handler is different than APIGatewayProxyResponseEvent. So if the wrapper mapping causes issues, the user can always switch to return the event. In other words both with input and result if the new wrapper breaks, user can implement own handler to input / return the event - and everything will work the same way it did (no mapping if no custom type used).

}
}
return event;
}
}
20 changes: 17 additions & 3 deletions .../src/main/java/io/opentelemetry/instrumentation/awslambda/v1_0/TracingRequestWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,32 @@
package io.opentelemetry.instrumentation.awslambda.v1_0;

import io.opentelemetry.sdk.OpenTelemetrySdk;
import java.util.function.BiFunction;

/**
* Wrapper for {@link TracingRequestHandler}. Allows for wrapping a regular lambda, not proxied
* through API Gateway. Therefore, HTTP headers propagation is not supported.
*/
public class TracingRequestWrapper extends TracingRequestWrapperBase<Object, Object> {
public TracingRequestWrapper() {
super();
super(TracingRequestWrapper::map);
}

// Visible for testing
TracingRequestWrapper(OpenTelemetrySdk openTelemetrySdk, WrappedLambda wrappedLambda) {
super(openTelemetrySdk, wrappedLambda);
TracingRequestWrapper(
OpenTelemetrySdk openTelemetrySdk,
WrappedLambda wrappedLambda,
BiFunction<Object, Class, Object> mapper) {
super(openTelemetrySdk, wrappedLambda, mapper);
}

// Visible for testing
static Object map(Object jsonMap, Class clazz) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised this non-API Gateway class (so not populating HTTP attrs) is mapping, does it need to?

Copy link
Author

Choose a reason for hiding this comment

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

With our wrapper requesting Object input type, AWS will (out of my testing):

  • invoke with value in case of primitive values
  • invoke with Map<String, Object> in case of custom types
    So in order to support pre-mapped custom types we need to map the Map into appropriate class.

try {
return OBJECT_MAPPER.convertValue(jsonMap, clazz);
} catch (IllegalArgumentException e) {
throw new IllegalStateException(
"Could not map input to requested parameter type: " + clazz, e);
}
}
}
45 changes: 18 additions & 27 deletions .../main/java/io/opentelemetry/instrumentation/awslambda/v1_0/TracingRequestWrapperBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,54 +6,45 @@
package io.opentelemetry.instrumentation.awslambda.v1_0;

import com.amazonaws.services.lambda.runtime.Context;
import com.fasterxml.jackson.databind.ObjectMapper;
import io.opentelemetry.sdk.OpenTelemetrySdk;
import io.opentelemetry.sdk.autoconfigure.OpenTelemetrySdkAutoConfiguration;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.function.BiFunction;

/**
* Base abstract wrapper for {@link TracingRequestHandler}. Provides: - delegation to a lambda via
* env property OTEL_INSTRUMENTATION_AWS_LAMBDA_HANDLER in package.ClassName::methodName format
*/
abstract class TracingRequestWrapperBase<I, O> extends TracingRequestHandler<I, O> {

protected static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
private final WrappedLambda wrappedLambda;

protected TracingRequestWrapperBase() {
this(OpenTelemetrySdkAutoConfiguration.initialize(), WrappedLambda.fromConfiguration());
private final Method targetMethod;
private final BiFunction<I, Class, Object> parameterMapper;

protected TracingRequestWrapperBase(BiFunction<I, Class, Object> parameterMapper) {
This conversation was marked as resolved.
Show resolved Hide resolved
this(
OpenTelemetrySdkAutoConfiguration.initialize(),
WrappedLambda.fromConfiguration(),
parameterMapper);
}

// Visible for testing
TracingRequestWrapperBase(OpenTelemetrySdk openTelemetrySdk, WrappedLambda wrappedLambda) {
TracingRequestWrapperBase(
OpenTelemetrySdk openTelemetrySdk,
WrappedLambda wrappedLambda,
BiFunction<I, Class, Object> parameterMapper) {
super(openTelemetrySdk, WrapperConfiguration.flushTimeout());
this.wrappedLambda = wrappedLambda;
}

private Object[] createParametersArray(Method targetMethod, I input, Context context) {
Class<?>[] parameterTypes = targetMethod.getParameterTypes();
Object[] parameters = new Object[parameterTypes.length];
for (int i = 0; i < parameterTypes.length; i++) {
// loop through to populate each index of parameter
Object parameter = null;
Class clazz = parameterTypes[i];
boolean isContext = clazz.equals(Context.class);
if (i == 0 && !isContext) {
// first position if it's not context
parameter = input;
} else if (isContext) {
// populate context
parameter = context;
}
parameters[i] = parameter;
}
return parameters;
this.targetMethod = wrappedLambda.getRequestTargetMethod();
this.parameterMapper = parameterMapper;
}

@Override
protected O doHandleRequest(I input, Context context) {
Method targetMethod = wrappedLambda.getRequestTargetMethod();
Object[] parameters = createParametersArray(targetMethod, input, context);

Object[] parameters = LambdaParameters.toArray(targetMethod, input, context, parameterMapper);
O result;
try {
result = (O) targetMethod.invoke(wrappedLambda.getTargetObject(), parameters);
Expand Down
45 changes: 45 additions & 0 deletions ...src/main/java/io/opentelemetry/instrumentation/awslambda/v1_0/TracingSqsEventWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.awslambda.v1_0;

import com.amazonaws.services.lambda.runtime.Context;
import com.amazonaws.services.lambda.runtime.events.SQSEvent;
import io.opentelemetry.sdk.OpenTelemetrySdk;
import io.opentelemetry.sdk.autoconfigure.OpenTelemetrySdkAutoConfiguration;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;

public class TracingSqsEventWrapper extends TracingSqsEventHandler {
This conversation was marked as resolved.
Show resolved Hide resolved

private final WrappedLambda wrappedLambda;
private final Method targetMethod;

public TracingSqsEventWrapper() {
this(OpenTelemetrySdkAutoConfiguration.initialize(), WrappedLambda.fromConfiguration());
}

// Visible for testing
TracingSqsEventWrapper(OpenTelemetrySdk openTelemetrySdk, WrappedLambda wrappedLambda) {
super(openTelemetrySdk, WrapperConfiguration.flushTimeout());
this.wrappedLambda = wrappedLambda;
this.targetMethod = wrappedLambda.getRequestTargetMethod();
}

@Override
protected void handleEvent(SQSEvent sqsEvent, Context context) {
Object[] parameters =
LambdaParameters.toArray(targetMethod, sqsEvent, context, (event, clazz) -> event);
try {
targetMethod.invoke(wrappedLambda.getTargetObject(), parameters);
} catch (IllegalAccessException e) {
throw new IllegalStateException("Method is inaccessible", e);
} catch (InvocationTargetException e) {
throw (e.getCause() instanceof RuntimeException
? (RuntimeException) e.getCause()
: new IllegalStateException(e.getTargetException()));
}
}
}
Loading