Submission Client Feedback
Closed this issue · 9 comments
Multiple http clients
Would be nice if there was only one submission client instead of having one for settings too All of our current clients follow this pattern (doesn't mean we have to, but it's good to be consistent).
Utils Class
String requestJSON = Utils.JSON_MAPPER.writeValueAsString(data);
I don't know if we need to have a JSON_MAPPER in the utils class. It feels like we could just do the serialization inline or use a ISerializer service on configuration. We know our serialization format and I don't know if it needs to be exposed.
Checking server response is success
We reworked the js client a bit in this front, but it seems like it would be better to check response.isSuccessful instead of response.code() / 100 != 2. Unless you are running into some kind of issue. Again you are the domain expert :).
Settings
We'll return a 304 if settings are current and won't return a body. Otherwise if the status is a 200 it will contain the configuration. This would simplify some checks :)
Throwing errors.
We typically return a model from the submission client to give the status (again not required), we do this because we don't want to throw exceptions that might go unhandled. Our goals in the client is to never blow up a clients application ever, just log out a failure happened. Would be good to ensure we are ensuring the exceptions we do throw are caught at the call sites.
Async (what we have now works)
In .NET and previous version of JavaScript client we have a fully sync client. For both of these we did this because at the time we wrote it async wasn't prevalent in the languages. When we rewrote the JavaScript client we made it async but it had a cascading virus affect to the code base. Since submissions happen in a background thread (queue timer) I still think it's fine being sync. I added this section only to consider it if it's gained main stream adoption in Java and if we feel like it's worth doing as it will be a breaking change later. But I feel if we do this we should do the whole api (queue processing, plugins etc). Is it worth it? Is it even possible... do timers support async safely (async over sync)?
Rate Limiting
We also return a rate limit header in addition to the config version. This could be fed into the queue submission to turn off the queue processing :-) https://github.com/exceptionless/Exceptionless.JavaScript/blob/feature/workspaces/packages/core/src/submission/SubmissionClientBase.ts#L17
https://github.com/exceptionless/Exceptionless.JavaScript/blob/feature/workspaces/packages/core/src/queue/DefaultEventQueue.ts#L210
We currently reset rate limits at the top of every 15 minutes on the hour (0, 15, 30, 45). Might be good to add some variability to this. Again this is not needed for v1 but a nice to have.
Multiple http clients
Would be nice if there was only one submission client instead of having one for settings too All of our current clients follow this pattern (doesn't mean we have to, but it's good to be consistent).
It will introduce cycle dependency between SettingsManager and SubmissionClient
Utils Class
String requestJSON = Utils.JSON_MAPPER.writeValueAsString(data);
I don't know if we need to have a JSON_MAPPER in the utils class. It feels like we could just do the serialization inline or use a ISerializer service on configuration. We know our serialization format and I don't know if it needs to be exposed.
We need this little bit of logic to work with java 8-time APIs.
public static final ObjectMapper JSON_MAPPER;
static {
JSON_MAPPER = new ObjectMapper();
JSON_MAPPER.registerModule(new JavaTimeModule());
JSON_MAPPER.disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS);
}
Checking server response is success
We reworked the js client a bit in this front, but it seems like it would be better to check response.isSuccessful instead of response.code() / 100 != 2. Unless you are running into some kind of issue. Again you are the domain expert :).
Settings
We'll return a 304 if settings are current and won't return a body. Otherwise if the status is a 200 it will contain the configuration. This would simplify some checks :)
👍
Async (what we have now works)
In .NET and previous version of JavaScript client we have a fully sync client. For both of these we did this because at the time we wrote it async wasn't prevalent in the languages. When we rewrote the JavaScript client we made it async but it had a cascading virus affect to the code base. Since submissions happen in a background thread (queue timer) I still think it's fine being sync. I added this section only to consider it if it's gained main stream adoption in Java and if we feel like it's worth doing as it will be a breaking change later. But I feel if we do this we should do the whole api (queue processing, plugins etc). Is it worth it? Is it even possible... do timers support async safely (async over sync)?
Timers are async
Throwing errors.
We typically return a model from the submission client to give the status (again not required), we do this because we don't want to throw exceptions that might go unhandled. Our goals in the client is to never blow up a clients application ever, just log out a failure happened. Would be good to ensure we are ensuring the exceptions we do throw are caught at the call sites.
Rate Limiting
We also return a rate limit header in addition to the config version. This could be fed into the queue submission to turn off the queue processing :-) https://github.com/exceptionless/Exceptionless.JavaScript/blob/feature/workspaces/packages/core/src/submission/SubmissionClientBase.ts#L17
https://github.com/exceptionless/Exceptionless.JavaScript/blob/feature/workspaces/packages/core/src/queue/DefaultEventQueue.ts#L210We currently reset rate limits at the top of every 15 minutes on the hour (0, 15, 30, 45). Might be good to add some variability to this. Again this is not needed for v1 but a nice to have.
This issue is stale