fix(AWSPinpoint): Connectivity errors should be retried indefinitely #4357
Conversation
|
@ruisebas Sorry for the flood of comments. Just making sure it's clear. There IS a quota of also testing the combined number of metrics and attributes as I wrote in my last comment it's confirmed by AWS support. So you should probably not remove the check as the PR currently does but rather adjust it to what PinPoint would actually check. (And again, it's best if you reproduce on your end to confirm the behaviors when surpassing the separate/mutual quotas). I can confirm the scenarios I tested but I didn't try to send 40 attributes and 40 metrics. The issue discussion has separate parts based on what I know vs what I was guessing. Last comment mostly relevant for this PR - adjusting the condition to be <=. |
AWSPinpoint/AWSPinpointEvent.m
Outdated
| @@ -115,12 +110,12 @@ - (void)addAttribute:(NSString *)theValue forKey:(NSString *)theKey { | |||
| if(!theKey) return; | |||
|
|
|||
| @synchronized(self.attributes) { | |||
| if(self.currentNumOfAttributesAndMetrics < MAX_NUM_OF_METRICS_AND_ATTRIBUTES) { | |||
| if(self.attributes.count < MAX_NUM_OF_METRICS_AND_ATTRIBUTES) { | |||
There was a problem hiding this comment.
I think the correct quota check would be the same as before BUT the comparison should be <=. Also the same warning applies.
There was a problem hiding this comment.
I'm waiting for someone from Pinpoint to confirm which one is the right limit, as I was able to successfully submit events with 40 metrics and 40 attributes.
Note that when this condition evaluates to true, we add a new attribute.
So the check needs to be < regardless, otherwise we would accept one extra attribute when the event is already at its capacity limit, which would result in Pinpoint rejecting its submission.
The same applies for the addMetric method below.
There was a problem hiding this comment.
Thanks for the clarification I understand now why the condition is < and not <=.
AWSPinpoint/AWSPinpointEvent.m
Outdated
| @@ -160,11 +155,11 @@ - (void)addMetric:(NSNumber *)theValue forKey:(NSString *)theKey { | |||
| } | |||
|
|
|||
| @synchronized(self.metrics) { | |||
| if(self.currentNumOfAttributesAndMetrics < MAX_NUM_OF_METRICS_AND_ATTRIBUTES) { | |||
| if(self.metrics.count < MAX_NUM_OF_METRICS_AND_ATTRIBUTES) { | |||
There was a problem hiding this comment.
I think the correct quota check would be the same as before BUT the comparison should be <=. Also the same warning applies.
ruisebas
force-pushed
the
ruisebas/analytics_offline_submit
branch
from
078d961
to
bf00b67
Compare
…ws-amplify#4357) Also: - Updating Pinpoint event quotas and exposing them as public constants - Removing NSKeyedUnarchiver warnings
Issue #, if available:
Description of changes:
retryCountwon't be increased.NSKeyedUnarchiverdue to missing allowed classes.Check points:
Added new tests to cover change, if neededAll integration tests passDocumentation update for the change if requiredBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.