search

amplitude / Amplitude-Node

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

Add Transport class to do http request #8

Closed haoliu-amp closed 4 years ago

haoliu-amp commented 4 years ago

A few code style things but a couple of important functional issues as well.

  • Thinking about process shutdown is really import
  • I think we need to look at the queuing design taking into account that the server might be logging on behalf of multiple client devices.

I got the 1st one. What's the concern for the 2nd one? Could you explain more?

trashstack commented 4 years ago

The second point is related to this comment: https://github.com/amplitude/Amplitude-Node/pull/8#discussion_r437096071

When we send events to the API it's important that we send them in order for each device id. On client SDKs that's simple because all the events belong to that device id. For a server SDK we will probably be sending events for lots of different device ids. Some of those device ids might get throttled so we'll need to queue up events for them. While this is happening we shouldn't hold up events that are being logged on different device ids.

haoliu-amp commented 4 years ago

Some of those device ids might get throttled so we'll need to queue up events for them. While this is happening we shouldn't hold up events that are being logged on different device ids.

Our backend throttle the event payload based on deviceId?

Let's say my payload contains events from different deviceIds. One deviceId gets throttled, and the whole payload won't be submitted?

trashstack commented 4 years ago

I don't know. Perhaps?