Skip to content
This repository has been archived by the owner on Mar 9, 2018. It is now read-only.

Add reflection for GCM NotRegistered errors, and other errors that imply an invalid registration id. #171

Conversation

marcrohloff
Copy link
Contributor

No description provided.

@jakeonfire
Copy link
Contributor

Very interested in using your patch - wish the specs weren't failing! Maybe I should just install your forked code...

@marcrohloff
Copy link
Contributor Author

Looking at the Travis build history its failing across all pull requests not just mine. If there is something specific to my patch I'd be happy to fix it.

ileitch added a commit that referenced this pull request Nov 11, 2013
…gistered-errors

Add reflection for GCM NotRegistered errors, and other errors that imply an invalid registration id.
@ileitch ileitch merged commit 73045a7 into ileitch:master Nov 11, 2013
@jakeonfire
Copy link
Contributor

I am trying to instrument Rapns via NewRelic using the most recent code, specifically to record counts of success and failure rates for notification-recipients. There is one case that I am having trouble with. I was hoping you could correct my assumptions or guide me in the right direction. When a GCM batched notification is marked as failed and Reflections#notification_failed is called, the recipients fall into one of three groups:
# 1. delivered successfully
# 2. failed to deliver due to invalid registration id (errors: 'InvalidRegistration', 'MismatchSenderId', 'NotRegistered', 'InvalidPackageName')
# 3. failed to deliver due to any other error
It is my understanding that those that fall into # 1 are nearly impossible to count using the reflection API alone, as the batch notification's status is only successful if all recipients within are successful, but if some are successful and some failed there is no direct reflection for the success subset. Am I missing something?

Before this pull request it was possible (though not ideal) to get this number by subtracting errors (parsed from the error string) from total recipients in a failed notification. Now that they are split into three groups this is no longer possible.

For our Rapns use case, at least, it would be more useful to have success and failure (and enqueue perhaps) reflections called for individual notification-recipients for batched GCM notifications. Currently it is somewhat inconsistent...

@marcrohloff
Copy link
Contributor Author

I would add a line below the multi_json_load line in Rapns::Daemon::Gcm::Delivery#ok and extract the values there (ideally using a reflection). It would be good to create a matching reflection for Apn deliveries.

@jakeonfire
Copy link
Contributor

I was thinking about how to implement your suggestion and came across another issue:
Looking at the code, it seems if 1) the delivery fails and 2) all the errors are one of the INVALID_REGISTRATION_ID_STATES, it looks like the notification does not get marked as failed, delivered, or retry(able). I see there is a spec basically proving this (it 'does not retry, raise or marks a notification as failed for invalid ids.') but wouldn't it then get picked up by the daemon again ad infinitum?
Also, if the notification handling throws a Rapns::DeliveryError, we never get to handle_canonical_ids.
Perhaps Rapns::Daemon::Gcm needs to be re-thought and re-organized? I don't think the attempt to create parity between the GCM and APN reflection APIs is helping things - the batching just makes them too dissimilar.

@marcrohloff
Copy link
Contributor Author

The way I understand it from this http://developer.android.com/google/gcm/adv.html#canonical, these messages are still delivered. I didn't really change this path except to add handle_canonical_ids at the end.
I'll have to think through the first issue.

@jakeonfire
Copy link
Contributor

As to the second point, if a batch has both canonical_ids and non-invalid-registration_id failures, it throws a DeliveryError in handle_errors() and never gets to handle_canonical_ids(), right? This has nothing to do with this pull request - just bouncing it off you because you seem knowledgeable :o)

@marcrohloff
Copy link
Contributor Author

I made a pull request to fix this. Do you mind taking a look? #180

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.

3 participants