-
Notifications
You must be signed in to change notification settings - Fork 375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FR]Use multiplexing over HTTP2 in sendEachForMulticast method for batch push. #2488
Comments
I found a few problems with this issue:
|
@lahirumaramba, @jonathanedey Instead of just using HTTP/2 multiplexing I strongly advocate restoring the There is not much in computer science that can be said accross the board. But one of the few exceptions is, that IO operations, especially network requests, are slow compared to CPU bound operations. That's a fact, every software developer should know. While it might be acceptable for small hobby projects to use It's a standard use case for us to send notifications to thousands of devices that are subscribed to some topics. While this was no problem with the legacy API, it will make thousands (!) of HTTP requests with the new API taking up seconds to minutes instead of milliseconds. That doesn't only mean that user experience get's worse, due to delayed notifications, it also means, that our costs rise by a factor of 10, 15 or even more, as we are using serverless infrastructure that is billed by the millisecond. Even if you don't care about performance and costs, this is a massive waste of resources. I honestly believe it is the task of every developer and software architect to be as efficient as possible with resources. Especially in the times, we live in. => It is therefore safe to say that this is a severe design error in the HTTP API v1. And just in case you want to point out, that we should send to topics instead devices: that doesn't work either. Because it doesn't support the use case where we need to address users, that are interested in either one of multiple topics without sending duplicate notifications. (I don't even want to open a feature request suggesting this, as it is pretty much pointless, considering the other issues here.) Also, the option mentioned in the docs, to use I really can't understand why you make these decisions and not listen to your users. It's very frustrating. |
Hey @MightySepp666 the support for batch send APIs including In the meantime, I'd recommend filing a Firebase support ticket from https://firebase.google.com/support/troubleshooter/contact to share your feedback on the above to reach the correct internal teams. |
Hope this gets prioritized soon. Critical issue IMO. |
I am not a serious node programmer, just want to simply replace the "sendMulticast" method to another one if deprecated. And don't want to shut out my server through using the super slow replacement. I am sure 95% server maintainers have no idea that 1 month later their server will be crashed, after changing sendMulticast => sendEachForMulticast. I am just waiting for the crash going to be happening. As I don't really know how to resolve this situation actually. |
@MightySepp666 Like you, I don't understand this step backwards. The Also, the topics system isn't usable because it involves sending exactly the same notification to users, but in most cases, the payload of each notification needs to be customized according to the person for whom the notification is intended, so it doesn't work. I opened a ticket as suggested by @lahirumaramba to contact support. If you find a solution that improves performance I'm interested... |
@LebonNic I already opened a ticket as suggested some weeks ago. Hopefully you'll have better luck than me. To me it seems like Firebase is not interested in customer feedback (even if this most likely also increases their operating costs as well). An interesting thing to mention is, that I've observed that the performance of the The |
Using this package may be an option if you cannot afford to wait for longer and need a workaround. Have not tested it myself so cannot vouch for how well it works. Ideally, the Firebase team should fix this as soon as possible. |
@jimnor0xF thanks for the tip. But I looked at the code and it would be rather cumbersome for me to integrate. It's not a drop-in replacement and doesn't provide the other API methods of the Firebase SDK. Also I would require to patch it, like the regular Firebase SDK, due to the still open bug of duplicate push notifications caused by the retry functionality (see #1900 (comment)). It doesn't export types as well, so it's harder to integrate with TypeScript. At the moment I am rather considering replacing Firebase completely by a different push notification provider backend. |
I faced this issue too. Can you suggest another Firebase alternative service? |
All users and businesses using the Firebase Admin SDK are experiencing the same issue. |
Any progress regarding this topic? |
Thank you for your patience! With the release of v12.3.0, While we work through this transition, if you are facing any issues with the new HTTP/2 transport, we have provided access to the legacy HTTP/1.1 versions of these methods. This can be enabled by using the |
There is no support for HTTP/2 multiplexing yet, so notifications are sent over a single HTTP/2 connection, which is limited to 100 concurrent streams (requests). This means that only 100 notifications can be sent at any given time over this single connection, which makes firebase-admin unsuitable for use cases that require high throughput (>100 notifications per second). Any estimate for when this will be available? |
Hey @victorHugoCarvalho, If the issue here is having a 1 to 1 ratio of streams to messages this can be done by batching your If this isn't the behaviour you are experiencing please open a new issue and we can take a look there. |
Hey @jonathanedey, The problem we are facing is precisely the delay in sending these notifications. Overall we are making 450 calls to sendEachForMulticast() where each batch has 500 tokens. |
The "sendMulticast()" function has been deprecated, and in fact the way this function used to work is no longer supported server-side by Firebase. This was causing sendMulticast() to fail *silently*, since it doesn't throw exceptions upon failure, but returns a list of successful and unsuccessful responses. In our code, we don't really check the reason for the failure, and just proceed to invalidate the associated device token, assuming that it's no longer valid. This updates our code to use the newly recommended sendEachForMulticast() function, which is not exactly a drop-in replacement, since it doesn't actually send a single multicast message, but rather multiple messages. Nevertheless it uses an HTTP/2 connection, so performance should not be impacted significantly. This also adds a little more logging upon failure, and bumps the minimum version of the Firebase dependency, and tidies up tests. It's not really clear what exactly the test was testing (since it didn't catch this failure). Expanding tests is something to visit in the future. https://firebase.google.com/support/release-notes/admin/node#version_1170_-_18_april_2023 firebase/firebase-admin-node#2488 Bug: T379068 Change-Id: I6db51687bf1a16c15a274a21df1d472c2b349ea9
Is your feature request related to a problem? Please describe.
A clear and concise description of what the problem is.
When I upgrade fcm to Http V1, the speed will become very slow when using sendEachForMulticast in firebase-admin:9.2.0.
Isn't HTTP/2 used in the SDK?
Describe the solution you'd like
Convert bulk send in SDK to HTTP/2.
Describe alternatives you've considered
Keep the original sendMulticast method.
Additional context
According to the database conditions, users (hundreds of thousands) who meet the sending conditions are selected for message push.
If the current sendEachForMulticast is used, it will be a nightmare.
The following code took 15 seconds to execute.
The text was updated successfully, but these errors were encountered: