firebase / firebase-admin-node

Firebase Admin Node.js SDK
https://firebase.google.com/docs/admin/setup
Apache License 2.0
1.64k stars 372 forks source link

[FR]Use multiplexing over HTTP2 in sendEachForMulticast method for batch push. #2488

Open zoran-lee opened 9 months ago

zoran-lee commented 9 months ago

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 commented 9 months ago

I found a few problems with this issue:

MightySepp666 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

Hope this gets prioritized soon. Critical issue IMO.

TaejunPark commented 7 months ago

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 commented 6 months ago

@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 commented 6 months ago

@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 https://github.com/firebase/firebase-admin-node/issues/1900#issuecomment-2014853972). 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 commented 6 months ago

@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 commented 6 months ago

@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 https://github.com/firebase/firebase-admin-node/issues/1900#issuecomment-2014853972). 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 commented 6 months ago

@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 commented 6 months ago

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. https://github.com/firebase/firebase-admin-java/issues/941

victorHugoCarvalho commented 5 months ago

Any progress regarding this topic?

jonathanedey commented 4 months ago

Thank you for your patience! With the release of v12.3.0, sendEach() and sendEachForMulitcast() now use a HTTP/2 connection by default when sending messages. We'd like for you to give it a try and give us your feedback on these changes.

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 enableLegacyTransport() method. This method is already marked as deprecated and will be removed once the HTTP/2 transport is considered fully stable.

victorHugoCarvalho commented 2 months ago

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?

jonathanedey commented 2 months ago

Hey @victorHugoCarvalho, The current implementation does use HTTP/2 multiplexing. Each call to sendEach() or sendEachForMulitcast() creates a new connection with 100 concurrent streams. Meaning 2 sendEach() calls would result in 2 HTTP/2 connections with 200 concurrent streams and 200 messages in flight at a time.

If the issue here is having a 1 to 1 ratio of streams to messages this can be done by batching your sendEach() calls with 100 messages each rather than the allowed maximum of 500.

If this isn't the behaviour you are experiencing please open a new issue and we can take a look there.

victorHugoCarvalho commented 2 months ago

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.