AWS lambda - improvements in custom type handling in wrappers, SQS ev… #4254
Conversation
ghost
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
...src/main/java/io/opentelemetry/instrumentation/awslambda/v1_0/TracingRequestWrapperBase.java
Show resolved
Hide resolved
...ry/src/main/java/io/opentelemetry/instrumentation/awslambda/v1_0/TracingSqsEventWrapper.java
Outdated
Show resolved
Hide resolved
...ry/src/main/java/io/opentelemetry/instrumentation/awslambda/v1_0/TracingSqsEventWrapper.java
Show resolved
Hide resolved
.../library/src/main/java/io/opentelemetry/instrumentation/awslambda/v1_0/LambdaParameters.java
Outdated
Show resolved
Hide resolved
.../library/src/main/java/io/opentelemetry/instrumentation/awslambda/v1_0/LambdaParameters.java
Show resolved
Hide resolved
| } else { | ||
| try { | ||
| event = new APIGatewayProxyResponseEvent(); | ||
| event.setBody(OBJECT_MAPPER.writeValueAsString(result)); |
There was a problem hiding this comment.
Do we know that our OBJECT_MAPPER is configured the same as Lambda?
There was a problem hiding this comment.
Unfortunately no guarantees here. Haven't seen an official spec by AWS guys regarding how the JSON is mapped in their code.
| event = new APIGatewayProxyResponseEvent(); | ||
| event.setBody(OBJECT_MAPPER.writeValueAsString(result)); | ||
| } catch (JsonProcessingException e) { | ||
| throw new IllegalStateException("Could not serialize return value.", e); |
There was a problem hiding this comment.
These exceptions seem worrisome, are we sure we won't break the app?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
…lemetry/instrumentation/awslambda/v1_0/LambdaParameters.java Co-authored-by: Anuraag Agrawal <[email protected]>
…lemetry/instrumentation/awslambda/v1_0/LambdaParameters.java Co-authored-by: Anuraag Agrawal <[email protected]>
...est/groovy/io/opentelemetry/instrumentation/awslambda/v1_0/TracingSqsEventWrapperTest.groovy
Outdated
Show resolved
Hide resolved
| event = new APIGatewayProxyResponseEvent(); | ||
| event.setBody(OBJECT_MAPPER.writeValueAsString(result)); | ||
| } catch (JsonProcessingException e) { | ||
| throw new IllegalStateException("Could not serialize return value.", e); |
There was a problem hiding this comment.
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?
| } | ||
|
|
||
| // Visible for testing | ||
| static Object map(Object jsonMap, Class clazz) { |
There was a problem hiding this comment.
I'm surprised this non-API Gateway class (so not populating HTTP attrs) is mapping, does it need to?
There was a problem hiding this comment.
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.
...rary/src/test/java/io/opentelemetry/instrumentation/awslambda/v1_0/LambdaParametersTest.java
Outdated
Show resolved
Hide resolved
…en wrapper added
Two goals with this PR: