-
Notifications
You must be signed in to change notification settings - Fork 38
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
Trap errors from the export endpoint #31
base: master
Are you sure you want to change the base?
Conversation
15bc7c5
to
616205e
Compare
Is this working? I switched to this branch and still get untrapped errors. Or if this is working, are you going to merge it? |
Here's what my error logs look like: |
@GeoffreyPlitt I'll need to put aside some time to look back into this for you. I'm not saying it's the case here, but historically I've had issues with Mixpanel not consistently returning valid JSON (or even consistent error messages), their export API is prone to breaking changes. Could you give me some info on the request you're making so I can recreate this locally please? I can then either put in a fix, or add some pointers on here for someone else to put in a PR. |
Gotcha. We're doing mixpanel_exporter.export() with an opts object like this: {
event: ['ev1', 'ev2', 'ev3'],
from_date: '2019-03-01',
to_date: '2019-03-05',
where: `"44845" in properties["field1"]`
} |
Another one
|
Mixpanel's non-export API endpoints give 200 status codes with a JSON error when something goes wrong, but the export endpoint gives a non-200 status code with a string. The library was trying to parse this into JSON, failing awkwardly and blowing up.
This isn't really a full fix, as ideally we'd have promises rejecting properly with errors when they occur, but it should at least make the export API's behaviour consistent with behaviour of the other methods and give a similarly structured
{ error: "foobar" }
object