amplitude / Amplitude-Node

Server-side Node.js SDK for Amplitude
MIT License
67 stars 20 forks source link

logEvent - Catch HTTP status of the request as a promise #12

Closed axelvaindal closed 4 years ago

axelvaindal commented 4 years ago

Hello,

Thanks for providing a node SDK for developers. I have been working with it lately, but it was very hard to debug the request because I didn't manage to catch the status of the HTTP request made during logEvent.

Is there any way to return a promise from the logEvent function so we can do debug better when we integrate with Amplitude ?

Thanks for your help 🙂

haoliu-amp commented 4 years ago

@axelvaindal Glad to hear that you're using our Node SDK. Are you trying to know if an event was successfully submitted to our sever?

axelvaindal commented 4 years ago

@haoliu-amp Indeed, this is what I'm trying to do. More specifically, I would like to get the success status if it's okay or the error message to help me debug the current logEvent request if it's not.

haoliu-amp commented 4 years ago

We're thinking changing current design. Instead of triggering a network call whenever submitting one event. We wanna buffer events and then create a batch request instead over all these events.

So when you log an event, there won't be any errors since it's just putting it to a buffer.

However, when batch request happens, we can expose a promise that time, and you can listen to that. Do you think that will help?

axelvaindal commented 4 years ago

When exactly will the buffer be executed ? Are you thinking about providing something like that:

client.logEvent(); // this push inside an internal buffer
client.logEvent(); // this push inside an internal buffer
await client.sendEvents(); // This send the batch request with all the events in the buffer

Or do you think the batch send will be handled automatically based on a specific trigger ?

Considering there may be multiple events in that batch, I assume you could expose a single Promise, which either return a success status (or whatever) if the batch request went well, or the array of errors description for each event that went wrong 🤔

haoliu-amp commented 4 years ago
  1. buffer execution time will be decided by the threshold you set, like every 30 sec etc.

  2. Batch will be automatic and it can be forced to flush too

  3. Yes, single promise.

axelvaindal commented 4 years ago

Alright, things are clear thanks.

Not sure if I will ever need to batch event sending, but if you feel this is needed to avoid spamming your service with single calls, it's fine for me. In our current usage, we simply log a single event when something occurs in our server, so the call will never contain multiple events, but perhaps there may be some cases when someone would like to log multiple times in the same server side process.

BeatrizFerreira commented 4 years ago

Firstly, thanks for the new SDK for Node :) I was about to open a new issue, but I guess this issue is exactly the problem we have. We want to log some events on Amplitude by executing a cloud function (google cloud function) written in node. The thing is: when the cloud function is invoked, one instance of it is created, executed and, when it finishes, the instance is destructed by GCP and we won't be able to check if the request was successful or not. I believe having a Promise as the return from the logEvent method would help to prevent loss of events by resending them or logging the error when it happens.

Is there any way of returning a Promise from logEvent method?!

I've seen your team already have an idea of a new architecture solution that would be great for sending lots of events in one roll, but I'd suggest having also the possibility of sending only one event ( that would be very useful for simple log events using node on server-side).

Thanks in advance.

haoliu-amp commented 4 years ago

@BeatrizFerreira Thanks for your feedback! We will definitely consider to keep that API with returning a Promise!

cc: @kelvin-lu

BeatrizFerreira commented 4 years ago

@haoliu-amp that is great! Looking forward to the new version with this change!

kelvin-lu commented 4 years ago

@BeatrizFerreira @axelvaindal this is now available in 1.0.3 - see the response type to see what logEvent and flush return