Event aggregation logic for reviewers #74
Conversation
Runtime/Services/BacktraceSession.cs
Outdated
| { | ||
| OnRequestCompleted(); | ||
| } | ||
| else |
There was a problem hiding this comment.
On first read, it seems that this doesn't try with backoff.
There was a problem hiding this comment.
Since we're not in API that allows us to use coroutines (mono behavior) we can't, for example, wait for n seconds without blocking the main thread. We shouldn't also use threads because we previously agreed that this will increase resources that backtrace-unity integration consumes and might impact games with a small pc budget. unfortunately, I can't have a better solution for exponential backoff than trying instantly just after the previous request :(
There was a problem hiding this comment.
Given the volume in question, we can't afford for these to spin out if the service goes down. We can wait for @wca to chime in if we want but this will effectively end up DDOSing ourselves one day. if we have to use a thread, then we probably have to use a thread. Maybe we can only do it when automatic submission is enabled?
I see that the interface for requests uses a callback; do we have a way to also do callbacks on timers, e..g JS setTimeout or something?
There was a problem hiding this comment.
Hey. I've moved this code to tick/update method. Tick/Update method will be invoked by Untiy engine every time when unity generates a new frame. So tick method will occur every time when unity will try to render a new frame. With that being said without an additional thread/timer we can use the update method to schedule jobs to be sent by BacktraceSession.
Now I added default timeout which is 10 seconds. Every time when we failed we're waiting at least 10 seconds for the next retry. Also like previously - we're making only 3 requests.
@ahicks92 what do you think about this solution?
Runtime/Services/BacktraceSession.cs
Outdated
| const int jitterFraction = 1; | ||
| const int backoffBase = 10; | ||
| var value = DefaultTimeInSecBetweenRequests * Math.Pow(backoffBase, numberOfRetries); | ||
| var retryLower = Math.Max(0, Math.Min(RetryTimeMax, value)); |
There was a problem hiding this comment.
Moved it to our internal helpers - Unity Clamp doesn't support doubles.
Runtime/Services/BacktraceSession.cs
Outdated
| var value = DefaultTimeInSecBetweenRequests * Math.Pow(backoffBase, numberOfRetries); | ||
| var retryLower = Math.Max(0, Math.Min(RetryTimeMax, value)); | ||
| var retryUpper = retryLower + retryLower * jitterFraction; | ||
| return new System.Random().NextDouble() * (retryUpper - retryLower) + retryLower; |
There was a problem hiding this comment.
Fine if we don't have one, but this would be clearer if there were a version of random that took the endpoints of the range, rather than requiring us to scale.
There was a problem hiding this comment.
Unity uniform function require to use floats. Here we have doubles. So I prefer to stay with this implementation instead of just casting it to floats.
There was a problem hiding this comment.
Moved to Internal helpers (like Clamp)
Runtime/Services/BacktraceSession.cs
Outdated
| return payload; | ||
| } | ||
|
|
||
| private double CalculateNextRetryTime(uint numberOfRetries) |
There was a problem hiding this comment.
Moved it closer to the Send method.
| /// <summary> | ||
| /// Force BacktraceSession to Send data to Backtrace | ||
| /// </summary> | ||
| public void Send() |
There was a problem hiding this comment.
I'm not a fan of how we're doing recursion by overloading a function of the same name 3 times. I'd consider giving these unique names.
There was a problem hiding this comment.
How about renaming Send method with unique/session events parameter to SendPayload?
Runtime/Services/BacktraceSession.cs
Outdated
| { | ||
| OnRequestCompleted(); | ||
| } | ||
| else if (statusCode == 503) |
There was a problem hiding this comment.
There's a pretty good chance this will be "retry on all 5xx" instead.
There was a problem hiding this comment.
statusCode > 501 && statusCode != 505
Runtime/Services/BacktraceSession.cs
Outdated
| else if (statusCode == 503) | ||
| { | ||
| _numberOfDroppedRequests++; | ||
| if (numberOfRetries - 1 != 0) |
There was a problem hiding this comment.
This code is where you want your retry counter to go up instead of down. If it did, you can just compare directly to defaultNumberOfRetries after incrementing, and pass directly to CalculateNextRetryTime. it seems like it would be easy to forget whether or not the number of retries is going up or going down, and almost anything interesting you want to do with retry counters needs them to be going up so I'd rather see us standardize on that.
There was a problem hiding this comment.
renamed to attemps and now we're increasing variable (not decreasing it like previously)
|
Approved pending changing the last place where we're flipping attempts which I discussed on slack. |
* Divided commit into two branches for reviewers. Added attribute provider and Backtrace http client for testing purpose. * Adjusted labels * Start default event on the application startup + send empty string instead of null * Event aggregation logic for reviewers (#74) * Event aggregation logic for reviewers * Adjusted the most flexible cases * Code review suggestions * Default event support * backoff for session events when request failed * Retry only on 503 * Exponential backoff * Avoid casting to float, fixed Math.Pow * Added Rameez suggestions * Code review suggestions * Fixed wrong retry function argument * line ending fix * Fixed invalid property name after merge * Adjusted nullable JSON values * Create attribute provider dynamically * Enable attribute provider * Disable maximum number of events in store from config * Adjust session aggregation support to spec requirements and meeting suggestions * Fixed typo * git attributes for line endings * Adjust current flow to new requirements + editor changes * Allow for overriding default unique event * Added getter and setter for backtracesesion support * Adjust backtrace session object * Backtrace session API improvements * Safe native attribute ios * Safe native attributes * Adjusted native build * Use empty list of classifiers instead of empty string * Adjusted API * Adjusted API + Session=>Summed + BacktraceSession=>BacktraceMetrics * Adjusted API + Session=>Summed + BacktraceSession=>BacktraceMetrics * Renamed ms to sec
* Divided commit into two branches for reviewers. Added attribute provider and Backtrace http client for testing purpose. * Adjusted labels * Start default event on the application startup + send empty string instead of null * Event aggregation logic for reviewers (#74) * Event aggregation logic for reviewers * Adjusted the most flexible cases * Code review suggestions * Default event support * backoff for session events when request failed * Retry only on 503 * Exponential backoff * Avoid casting to float, fixed Math.Pow * Added Rameez suggestions * Code review suggestions * Fixed wrong retry function argument * line ending fix * Fixed invalid property name after merge * Adjusted nullable JSON values * Create attribute provider dynamically * Enable attribute provider * Disable maximum number of events in store from config * Adjust session aggregation support to spec requirements and meeting suggestions * Fixed typo * git attributes for line endings * Adjust current flow to new requirements + editor changes * Allow for overriding default unique event * Added getter and setter for backtracesesion support * Adjust backtrace session object * Backtrace session API improvements * Safe native attribute ios * Safe native attributes * Adjusted native build * Use empty list of classifiers instead of empty string * Adjusted API * Adjusted API + Session=>Summed + BacktraceSession=>BacktraceMetrics * Adjusted API + Session=>Summed + BacktraceSession=>BacktraceMetrics * Renamed ms to sec * Event aggregation support (#79) * Divided commit into two branches for reviewers. Added attribute provider and Backtrace http client for testing purpose. * Adjusted labels * Start default event on the application startup + send empty string instead of null * Event aggregation logic for reviewers (#74) * Event aggregation logic for reviewers * Adjusted the most flexible cases * Code review suggestions * Default event support * backoff for session events when request failed * Retry only on 503 * Exponential backoff * Avoid casting to float, fixed Math.Pow * Added Rameez suggestions * Code review suggestions * Fixed wrong retry function argument * line ending fix * Fixed invalid property name after merge * Adjusted nullable JSON values * Create attribute provider dynamically * Enable attribute provider * Disable maximum number of events in store from config * Adjust session aggregation support to spec requirements and meeting suggestions * Fixed typo * git attributes for line endings * Adjust current flow to new requirements + editor changes * Allow for overriding default unique event * Added getter and setter for backtracesesion support * Adjust backtrace session object * Backtrace session API improvements * Safe native attribute ios * Safe native attributes * Adjusted native build * Use empty list of classifiers instead of empty string * Adjusted API * Adjusted API + Session=>Summed + BacktraceSession=>BacktraceMetrics * Adjusted API + Session=>Summed + BacktraceSession=>BacktraceMetrics * Renamed ms to sec * Fixed stucture size (#80) * Guess native library path when application context is not available (#81) * Guessing lib path based on the current game directory when the activity is not available * Code reformat * Renamed ms to sec * Handle different unity player java class name (#87) * Removed warnings generated by Unity2020 + Unity2021 (#85) * Fixed problem with Unity2021 - symbols upload warnings on the android upload symbols task * Removed warnings generated by Unity2020 * Fixed compatibility issues * Formatting * Metrics support imrovements (#88) * divided summed/unique events into different queues * Fully worked unit tests * adjusted url + json keys * Code review feedback * Breadcrumbs support (#83) * Breadcrumbs support: before database operations cleanup * Breadcrumbs improvements * Database adjustements * Adjusted database code * Removed try/catch block from function that handles exception and prevent from sending nullable records * Allow nullable record in SendData method * Squashed commit of the following: commit 8c2cec9 Author: kdysput <konrad.dysput@gmail.com> Date: Thu May 6 00:00:10 2021 +0200 Adjusted nullable JSON values commit 2ed2d7f Author: kdysput <konrad.dysput@gmail.com> Date: Wed May 5 23:55:47 2021 +0200 Fixed invalid property name after merge * Undo session aggregation changes * Create attribute provider dynamically * Undo maximum number of events * Undo attribute provider management * Fixed labels * Expose enableBreadcrumbs API * Fixed name * breadcrumbs tests * Breadcrumbs unit tests - breadcrumb file tests * Renamed enums * Log cleanup tests * Size consistency test * Add stack trace attribute only to error/exception logs * Adjusted native build * Safe native attributes * Safe native attribute ios * Fixed timestamp comparision * Fixed missing namespace + line endings * API update + removed two low memory warnings * breadcrumbs adjustements * Unit tests that validates navigation and logs events * Renamed AddBreadcrumb to Log * Send unique attachments (or include (#n) to attachment name) * Fixed condition + handling empty nullable files * Order beadcrumbs * breadcrumbs support - fixed ios side (delay breadcrumbs startup) * Removed database checks * added breadcrumbs.lastId + offline db support * removed early access flag * Fixed breadcrumb structure + unit tests * typos + new lines updates * Adjusted formating * Adjusted if statement * Removed extra line * COde cleanup - adjustemenets after CR * Fixed compilation issue * Don't dispose before checking network status * Use UTC timestamp * Removed universe ane token submission URL - allow user to override them (#89) * fixed line endings * Editor configuraiton improvements - foldout improvements * Apply default log file size * Label change * Final ios library * Use copy of attributes * Fixed reference shared between native client and managed client * applicaiton.session attribute * Limit number of events to 50 * Use breadcrumbs as double not long * Use only one variable to determinate maximum number of elements in queue * Setting maximum number of events per unique/summed events + breadcrumbId type change * Getter/Setter api interface improvements * Breadcrumbs improvements -> changed types + metrics expose maximum number of events + adjust api to new requiremenets * Unit tests for 502 status code + disabled auto send, long => uint api change, adjusted metrics initialization flow * Changed breadcurmb type: http => system for internet connection check * Read application version and name from cache to add support for background threads * Block WebGL * Calculate retry based on the time * version update * Update CHANGELOG.md * Update CHANGELOG.md * Update README.md * Update README.md * Update README.md * Update README.md * Update README.md * Update CHANGELOG.md Minor edits * TOC Update for Breadcrumbs and Crash Free Metrics * Ignore LogType.Error (#92) * Ignore errors * Adjusted comment * Screenshot management (#91) * Screenshot management * Changed default screenshot quality * Ratio typo * version bump: preview.2 update * Adjusted attachments + breadcrumbs support * Allow overriding default attributes * Add missing namespace * Application.session + api adjustements backtrace metrics * empty json object adjustement * Windows native dump event aggregation attributes * Correct unit test namespace * Convert device attributes to scoped attributes + include native attributes before native integration start * Breadcrumbs log level tooltip adjustement * Native upload handler tests * Version 3.5.0: Version update * Update README.md * Update README.md * Update CHANGELOG.md * Update README.md * Label change Co-authored-by: Vincent Lussenburg <vlussenburg@users.noreply.github.com> Co-authored-by: jasoncdavis0 <jdavis@backtrace.io>
This diff shows business logic responsible for event aggregation support in the Backtrace-Unity client.
Information for reviewers:
Code information for reviewers:
Why we remove session events after successful upload?.This diff contains only code - no technical descriptions, release announcement, readme/changelog changes. We will handle those changes in different diff once when we will be ready to ship all updates that we want to add to 3.5.0 to our customers