Skip to content

Don't swallow integration errors #60

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

Merged
merged 1 commit into from
Apr 23, 2018
Merged

Don't swallow integration errors #60

merged 1 commit into from
Apr 23, 2018

Conversation

f2prateek
Copy link
Contributor

As part of adding client side metrics, we want to propagate errors to analytics.js-core so that it can capture them.

https://paper.dropbox.com/doc/Analytics.js-Metrics-SDD-1hAD90lqGS4aZxHAHYPu7#:h2=analytics.js-integration

analytics.js-core has already been updated to capture errors when invoking integrations (segmentio/analytics.js-core#66).

As part of adding client side metrics, we want to propagate errors to analytics.js-core so that it can capture them.

https://paper.dropbox.com/doc/Analytics.js-Metrics-SDD-1hAD90lqGS4aZxHAHYPu7#:h2=analytics.js-integration

analytics.js-core has already been updated to capture errors when invoking integrations (segmentio/analytics.js-core#66).
@f2prateek
Copy link
Contributor Author

I should clarify that a.js-core hasn't been updated yet - the PR is in review, and I'll be merging this only after the a.js-core changes in segmentio/analytics.js-core#66 are merged.

@ccnixon
Copy link
Contributor

ccnixon commented Apr 16, 2018

Hey @f2prateek - we have this in place to ensure that if a single integration has a bug and errors, it won't cause the all other integrations to also crash. I think we definitely want to maintain this behavior.

@ccnixon
Copy link
Contributor

ccnixon commented Apr 16, 2018

Sorry disregard! I mixed up your commits.

Copy link
Contributor

@ccnixon ccnixon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@f2prateek f2prateek merged commit 33d97df into master Apr 23, 2018
@f2prateek f2prateek deleted the throw-errors branch April 23, 2018 22:49
f2prateek added a commit that referenced this pull request May 17, 2018
Same as #60 but for 2.x.

As part of adding client side metrics, we want to propagate errors to analytics.js-core so that it can capture them.

https://paper.dropbox.com/doc/Analytics.js-Metrics-SDD-1hAD90lqGS4aZxHAHYPu7#:h2=analytics.js-integration

analytics.js-core has already been updated to capture errors when invoking integrations (segmentio/analytics.js-core#66).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants