Skip to content
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

Open
zoran-lee opened this issue Mar 7, 2024 · 11 comments

Comments

@zoran-lee
Copy link

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.

        List<String> registrationTokens = new ArrayList<>();
        for (int i = 1; i <= 500; i++) {
            registrationTokens.add(TOKEN + i);
        }

        MulticastMessage message = MulticastMessage.builder()
            .setNotification(Notification.builder().setTitle("test title").setBody("test body").build())
            .addAllTokens(registrationTokens).build();
        BatchResponse response = FirebaseMessaging.getInstance().sendEachForMulticast(message);
        System.out.println(response.getSuccessCount() + " messages were sent successfully");
@google-oss-bot
Copy link

I found a few problems with this issue:

  • I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.
  • This issue does not seem to follow the issue template. Make sure you provide all the required information.

@MightySepp666
Copy link

@lahirumaramba, @jonathanedey Instead of just using HTTP/2 multiplexing I strongly advocate restoring the sendMulticast() method.

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 sendEachForMulticast() to send messages in separate network requests, is is surely not for a company that is sending approx. 16 million messages per month.

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 OR conditions, does also not work. It was obviously not built to scale. It works only for a very small number of topics, but with millions of topics, it takes many minutes for a single message to be delivered.

I really can't understand why you make these decisions and not listen to your users. It's very frustrating.

@lahirumaramba
Copy link
Member

lahirumaramba commented Apr 16, 2024

Hey @MightySepp666 the support for batch send APIs including sendMulticast have been deprecated in the backend services, which is beyond scope for this SDK. Firebase Admin Node SDK (this repo) is essentially a wrapper for the REST API. We are actively looking into implementing http/2 to efficiently use the new FCM send API for batch requests. We will use this issue to track any progress.

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.

@jimnor0xF
Copy link

Hope this gets prioritized soon. Critical issue IMO.

@TaejunPark
Copy link

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.
node firebase admin has been downloaded over 1 million per week. Do you guys think majority of those maintainers are smart enough to write their own batch codes?
Where are you spending my $1🧐☺️ paying for the firebase services?

I am just waiting for the crash going to be happening. As I don't really know how to resolve this situation actually.

@LebonNic
Copy link

@MightySepp666 Like you, I don't understand this step backwards. The sendAll method was introduced in the SDK because the use of individual send calls wasn't performing well enough. I understand that the maintainers of this repo can't do anything about it because they're not the ones in charge of developing the Firabse Messaging infrasctructure, but I think the problem should be brought to their attention.

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...

@MightySepp666
Copy link

@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 sendMulticast() is already extremely bad in our production environment - between 500ms to > 10 seconds (!) or more per call if you send to > 300 devices. This might be related to the bad response times of the Firebase backend since beginning of March (that is also causing problems with duplicate push notifications, as described in #1900 (comment)). But unfortunately, I don't have metrics of last year for comparison.

The sendEachForMulticast() adds additional latency on top. But interestingly, not linearly as I would have expected. That means, the deterioration factor is not as high as expected because the status quo is already quite bad.

@jimnor0xF
Copy link

@LebonNic @MightySepp666

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.

@MightySepp666
Copy link

@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.

@nvti
Copy link

nvti commented May 21, 2024

@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?

@honorhs
Copy link

honorhs commented May 21, 2024

All users and businesses using the Firebase Admin SDK are experiencing the same issue.
I don't understand why they are pushing to end support for the legacy API (batch API) without showing any interest in user feedback.
firebase/firebase-admin-java#941

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants