Skip to content
This repository has been archived by the owner on Aug 2, 2023. It is now read-only.

Switch to using System.Text.Json instead of Newtonsoft to improve performance. #778

Merged
merged 5 commits into from
Dec 2, 2021

Conversation

puneetg1983
Copy link
Contributor

With this PR, we are changing the default formatted for the RuntimeHost project from NewtonSoft to System.Text.Json (STJ). Few changes were done in the code because System.Text.Json had some limitations or is a bit stricter than NewtonSoft. They are:-

  1. STJ had a bug due to which KeyValuePairs are serialized in to camel case properly. This was KeyValuePair serialization doesn't follow camel case option in System.Text.Json dotnet/runtime#44385 and we had to update STJ to 6.0 to fix it.
  2. STJ requires that for serialization properties should be defined. It does not work with public fields only. That's why you see setter and getter added for some fields.
  3. The one issue that took the most of my time was classes inheriting from Attribute fail with a really bad error and this is a bug which is still open STJ failing ungracefully when attempting to serialize attributes dotnet/runtime#58947
  4. Multi dimensional arrays are not supported so we had to use the DataTableResponseObject to use jagged arrays.

This is still a risky PR because I cannot test all API combinations locally. I plan to test this out in staging environment once the PR is merged and if things don't look fine, I will revert the PR.

@nmallick1
Copy link
Member

@puneetg1983 , does the Response JSON change in anyway so as to require a change in swagger?

@puneetg1983
Copy link
Contributor Author

@puneetg1983 , does the Response JSON change in anyway so as to require a change in swagger?

I tested only the listdetectors and invoke detector call and ensured that they are exactly the same. But we have lot of other WEBAPI too and I dont know the best way to test all of them :(

@hforeste
Copy link
Contributor

@puneetg1983 , does the Response JSON change in anyway so as to require a change in swagger?

I tested only the listdetectors and invoke detector call and ensured that they are exactly the same. But we have lot of other WEBAPI too and I dont know the best way to test all of them :(

Hey @puneetg1983 , do we believe anything else to be different if ListDetectors and ExecuteDetector calls are fine?

@puneetg1983
Copy link
Contributor Author

@puneetg1983 , does the Response JSON change in anyway so as to require a change in swagger?

I tested only the listdetectors and invoke detector call and ensured that they are exactly the same. But we have lot of other WEBAPI too and I dont know the best way to test all of them :(

Hey @puneetg1983 , do we believe anything else to be different if ListDetectors and ExecuteDetector calls are fine?

Hey @hforeste , I do see a bunch of other controllers and we should validate once that they are working as expected. For e.g. I saw yesterday that the Observer controller is failing and thats because it was returning a JArray and that is expected to fail. I quickly glanced at other controllers and they dont seem to be using classes from NewtwonSoft but its better if we can check them all one.

@hforeste
Copy link
Contributor

Hey @puneetg1983 , added PR to remove the obsolete ObserverController. https://github.com/Azure/Azure-AppServices-Diagnostics/pull/780/files

@puneetg1983 puneetg1983 merged commit dcead36 into main Dec 2, 2021
@puneetg1983 puneetg1983 deleted the fixDateTimeOffsetBug branch December 2, 2021 19:31
puneetg1983 added a commit that referenced this pull request Dec 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants