Fix serialisation exception on default lambda events #4724
Conversation
acm19
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
|
|
| // First version to includes support for SQSEvent, currently the most popular message queue used | ||
| // with lambda. | ||
| // NB: 2.2.0 includes a class called SQSEvent but isn't usable due to it returning private classes | ||
| // in public API. | ||
| library("com.amazonaws:aws-lambda-java-events:2.2.1") | ||
|
|
||
| compileOnly("com.fasterxml.jackson.core:jackson-databind") | ||
| implementation("com.fasterxml.jackson.datatype:jackson-datatype-joda") |
There was a problem hiding this comment.
You have to be careful with adding implementation dependencies because they are usually shaded inside agent jar. Could you verify that it is not shaded into agent jar, if you need it shaded inside the agent then please explain why.
There was a problem hiding this comment.
I explained it here: https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/4724/files#r758320858. Please let me know that's clear and you want a comment in the code as well.
There was a problem hiding this comment.
I believe you are missing a few details here. aws-lambda-1.0/library is for manual instrumentation and aws-lambda-1.0/javaagent that is used for autoinstrumentaiton. The javaagent module depends on the library module. All runtime dependencies of javaagent module will be embedded inside javaagent jar under inst directory, check the files under javaagent/build/libs. During runtime classes from javaagent instrumentation are injected into the class loader that contains the library, but we only injects the so called helper classes (see https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/contributing/writing-instrumentation-module.md). Everything else is taken from the application class loader, so the content of jackson-datatype-joda that is embedded into agent jar isn't really used.
There was a problem hiding this comment.
What details exactly am I missing? The auto-instrumentation of lambda functions doesn't work well. It doesn't parse the default Amazon events, then the pull request. You need the jackson-datatype-joda in the agent so that it parses the events to the correct type defined in the instrumented lambda function. Please notice joda-time is transitively included by aws-lambda-java-events but jackson-datatype-joda isn't. Do you have a better idea on how to address the issue?
There was a problem hiding this comment.
I'm afraid of dependency conflict if we depend on a separate jackson library here - the function will have a version of jackson through aws-lambda-java-events, but this one may not be aligned and I think Jackson is always supposed to be aligned to risk problems.
Instead, we could add a compileOnly dependency on joda-time and vendor in just the parts of datatype-joda that we need, would that work?
There was a problem hiding this comment.
I'm afraid of dependency conflict if we depend on a separate jackson library here - the function will have a version of jackson through aws-lambda-java-events, but this one may not be aligned and I think Jackson is always supposed to be aligned to risk problems.
The version of Jackson is defined by the library's own dependencyManagement: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/dependencyManagement/build.gradle.kts#L33. That makes sure it's properly aligned with the Jackson Joda module.
Instead, we could add a compileOnly dependency on joda-time and vendor in just the parts of datatype-joda that we need, would that work?
I might not understand what you have in mind but I see 2 options, using the module or the approached mentioned by @kubawach in the other thread. I have preference for the module, I explained the reasons in the other thread. But I'm not a decision maker in this project and I'm happy to change it in order to get the issue fixed.
There was a problem hiding this comment.
The version of Jackson is defined by the library's own dependencyManagement
We use a compileOnly dependency on jackson-databind currently - this is to make sure the version of Jackson used is the one in the user's app / lambda framework dependency since we generally don't want instrumentation to control dependency versions in the app. So currently we aren't defining a version for Jackson here. It isn't hard for jackson-databind and jackson-joda to have inconsistent versions in the final app - simplest solution is probably to avoid jackson-joda
There was a problem hiding this comment.
this is to make sure the version of Jackson used is the one in the user's app / lambda framework dependency since we generally don't want instrumentation to control dependency versions in the app
The thing is, for the lambda layer, which is where this module is relevant, it doesn't matter if the app includes the dependency or not. Actually many lambdas don't include jackson and it still works. I think the important thing is that this dependency makes it into the lambda layer: https://github.com/open-telemetry/opentelemetry-lambda/tree/main/java/layer-wrapper.
So I could change it to compileOnly and testImplementation in this repository and make the changes where necessary so it gets included in the layer. If that works for you, please let me know where should we add that dependency so it gets added there in the way you prefer so that it's consistent with your way of working.
There was a problem hiding this comment.
Right - if an app doesn't include jackson, then it will be using the one included in the lambda runtime itself. I'm not sure but I guess this one
https://github.com/aws/aws-lambda-java-libs/blob/master/aws-lambda-java-serialization/pom.xml#L41
If jackson-databind used by the app / lambda runtime is 2.10.0 but we include 2.12.0, there is reasonable chance of a conflict causing breakage as the version alignment is outside our control. As instrumentation, we don't want to influence the version of non-OTel libraries as much as possible.
Since we only need to marshal a couple of joda types, I suspect we can add here a small amount of code to worry less about dependency conflicts, and it would probably have a reasonable effect on reducing added binary size.
There was a problem hiding this comment.
Sorry for the long silence, I've been off for holiday and was very busy when I came back. I've uploaded a version of the code without JodaModule.
| protected static final ObjectMapper OBJECT_MAPPER = | ||
| new ObjectMapper() | ||
| .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false) | ||
| .registerModule(new JodaModule()); |
There was a problem hiding this comment.
Adding JodaModule this way might not be a good idea. The problem here is that if the instrumented application does not contain JodaModule then the instrumentation can not be applied. Are you sure that all aws-lambda-java-core users also use jackson-datatype-joda?
There was a problem hiding this comment.
aws-lambda-java-events uses Joda: https://github.com/aws/aws-lambda-java-libs/blob/master/aws-lambda-java-events/pom.xml#L49. It uses it in ScheduledEvent for example. If you don't use JodaModule your lambda layer will fail to parse it properly as described in the bug report. Add that module shouldn't do anything apart from properly parsing classes using Joda. Joda is basically dead so hopefully AWS will release an updated version of the events with Java Time API but this project still needs to support older versions of the events library, so I'd expect it to remain.
There was a problem hiding this comment.
I think in the original PR that added the JSON deserialization I had commented on how it's a bit tricky that we aren't using AWS's same mechanism for deserialization, which presumably isn't using jackson-datatype-joda.
More worringly is that the intent was that the instrumentation wouldn't cause exceptions in the user's function if the serialization did have problems but that seems to be happening here. I think we need to fix that. Otherwise we may want to give up on handling JSON - Joda types are one corner case but it's hard to be confident we don't continue to run into more and more corner cases for this. @kubawach Do you happen to be around to provide thoughts on this?
There was a problem hiding this comment.
we aren't using AWS's same mechanism for deserialization, which presumably isn't using jackson-datatype-joda.
AWS might not even use Jackson I presume.
More worringly is that the intent was that the instrumentation wouldn't cause exceptions in the user's function if the serialization did have problems but that seems to be happening here. I think we need to fix that.
I agree but I think this issue needs fixing nevertheless as it's one of AWS standard events provided by their library. Not failing on missing properties is a step in this direction BTW.
Joda types are one corner case but it's hard to be confident we don't continue to run into more and more corner cases for this.
I think the instrumentation should parse all events in aws-lambda-java-events, so I wouldn't call it a corner case. I added tests for 3 of them but we can add test for all of them to avoid surprises.
There was a problem hiding this comment.
Hey, yes, @anuraaga, you remember well latest discussion on that and general skepticism regarding wrapper's ability to deserialize a required POJO from JSON.
My two cents here are:
-
In general the wrapper is a customer-centric "nice to have" component. Users can always use appropriate handler directly if the wrapper does not work in their particular case. POJO-handling wrapper is even more so. It has a built-in fallback mechanism - if user declares own lambda handler as <Object, Object> the wrapper will not try to map coming data. In such case it will work exactly as it worked before adding POJO mapping extension.
-
Interestingly AWS seems to publish the mapping code on GitHub - see https://github.com/aws/aws-lambda-java-libs/blob/master/aws-lambda-java-serialization/src/main/java/com/amazonaws/services/lambda/runtime/serialization/factories/JacksonFactory.java It's even in the M2 repo (albeit not up to date with changes on GitHub - last change 7 days ago, last release a year ago). So there could be a way to either implement the mapping same way as AWS does or even re-use their code by depending on the artifact.
To sum up:
- Adding
FAIL_ON_UNKNOWN_PROPERTIESwill certainly not hurt (AWS does the same). - Date/time conversion is done with reflection, we could use the same to avoid adding Joda etc (see https://github.com/aws/aws-lambda-java-libs/blob/master/aws-lambda-java-serialization/src/main/java/com/amazonaws/services/lambda/runtime/serialization/util/SerializeUtil.java)
- Overall strategy towards this can ultimately go one of two routes (as already signed by @anuraaga):
a) try to support everything same way as AWS (which may be tricky due to version changes) for example by using AWS artifact / re-using their mapping code
b) clearly state in the docs that POJO-mapping wrapper is aconvenience solutionthat may not support all cases (various types etc) in which case users should implement the mapping on the handler side (and declare it as <Object, Object>)
I personally lean towards option b) as it gives more control to users and relives the community from burden of supporting external (AWS) deserialization implementation.
There was a problem hiding this comment.
@acm19 ability to use the wrapper without code changes was the primary reason to introduce the changes we're discussing right now ;) However I side with @anuraaga that we may expect more corner cases in the future if we decide we (as a community) want to allow wrapper usage in each and every case. In this respect - this does not sound viable. But it's a "strategic" side of things. In this particular case I'd:
- set the option
FAIL_ON_UNKNOWN_PROPERTIES - implement types-via-reflection handling as documented in the AWS code
..but I'm not an approver so it's up to @open-telemetry/java-instrumentation-approvers to decide.
Wrt reflection and performance - I meant dynamic invocation in general, not exactly reflection. Static MethodHandles are common in the instrumentation code and known to be quite fast. Good way to avoid another dependency especially in the quite tirvial case.
There was a problem hiding this comment.
ability to use the wrapper without code changes was the primary reason to introduce the changes we're discussing right now
I think the discussion is not on the purpose but the usefulness. My point is, from the user perspective this is an essential feature. Not only for convenience but also because having to parse objects is not only an unnecessary hassle but impractical for the reasons I explained before. Again, not my call, but I think this feature should at least:
- Address all events POJOs in
aws-lambda-java-events. - Support edge cases as they arrive whenever sensible.
implement types-via-reflection handling as documented in the AWS code
..but I'm not an approver so it's up to @open-telemetry/java-instrumentation-approvers to decide.
If you prefer this way, I'm happy to change the PR accordingly. I think supporting "default" events is the essential bit missing that should be added in this PR anyway.
There was a problem hiding this comment.
Address all events POJOs in aws-lambda-java-events.
Support edge cases as they arrive whenever sensible.
This seems to make sense to me, thanks for discussing. The more event types we can cover in unit tests the safer it'll be.
There was a problem hiding this comment.
I'll be off for a few days, I'll update the PR with additional tests when I'm back and I'll address the rest of the comments. Thanks.
There was a problem hiding this comment.
I added SNS and S3 as well.
|
@anuraaga I've rebased it and added some Javadoc to the custom Joda module. |
Adds `FAIL_ON_UNKNOWN_PROPERTIES` to the paramter parser on the lambda as events coming from AWS have more fields than those represented in the libraries provided by AWS. Adds a custom `JodaModule` to the same parser to support `ScheduledEvent` which for now uses Joda Time. At the same time avoid the real Joda Module to not have extra dependencies. Resolves: #4645
…4724) Adds `FAIL_ON_UNKNOWN_PROPERTIES` to the paramter parser on the lambda as events coming from AWS have more fields than those represented in the libraries provided by AWS. Adds a custom `JodaModule` to the same parser to support `ScheduledEvent` which for now uses Joda Time. At the same time avoid the real Joda Module to not have extra dependencies. Resolves: open-telemetry#4645
Adds
FAIL_ON_UNKNOWN_PROPERTIESto the paramter parser on the lambdaas events coming from AWS have more fields than those represented in the
libraries provided by AWS. Adds
JodaModuleto the same parser tosupport
ScheduledEventwhich for now uses Joda Time.Resolves: #4645