Skip to content
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

can set maxEventsPerPage config option #767

Closed
wants to merge 1 commit into from
Closed

Conversation

MaxBittker
Copy link
Contributor

this needs to be noted in the docs but I don't want to have those pulled out before this ships.

I reset _sentErrors in _captureUrlChange and the pushState wrapper, which is a little bit janky but I believe it covers a wider range of applications without doing much harm. (although I could be misunderstanding the behavior of these apis)

@MaxBittker
Copy link
Contributor Author

fixes
#679
#530
#435
in a naive way that could leave something to be desired. (time-based throttling and per-fingerprint throttling)

@Sija
Copy link
Contributor

Sija commented Nov 15, 2016

IMHO this implementation is too naive to be useful. Usually you'd want to throttle only one kind of an error, and not all of them regardless of their type.

@MaxBittker
Copy link
Contributor Author

@Sija I don't necessarily disagree, might take another look at this tomorrow. interested in @benvinegar 's opinion here

@benvinegar
Copy link
Contributor

@Sija – the problem is that determining what is the "same" error is non-trivial. We could probably make a "best guess" effort from the client, but there's a risk that it could fail on seemingly similar errors.

That being said, we're going to explore such a solution, see what it looks like, and revisit this.

@Sija
Copy link
Contributor

Sija commented Nov 16, 2016

@benvinegar I agree, good starting point could be simple heuristics based on filename, lineno, colno and function properties.

@benvinegar
Copy link
Contributor

Now that #861 is enabled by default for everyone, I think we can come back to this. It's simple to implement, and there's an argument for having it in some situations (e.g. when pages are open in a tab for 24 hours straight).

One last ask: we should reset the captured error count whenever there's a URL change (e.g. via pushState).

@benvinegar
Copy link
Contributor

Heard from a user yesterday for whom they could benefit from this.

@beck
Copy link

beck commented Sep 8, 2017

IMHO this implementation is too naive to be useful

This would stop the bleeding.

For my use-case, the platform is a site builder and sometimes raven-js picks up a user's 3rd party failures. I'm brought to this ticket because the latest event resulted in ~40k events, which hits the rate limit, stats look like the platform is down, and legitimate events are dropped.

This would allow us to set an upper-bound on the pathological scenarios.

@kamilogorek
Copy link
Contributor

I don't see any particular reason why we'd not want to include this config option (with it being set to Infinite by default). What do you think about it now @benvinegar?

@benvinegar
Copy link
Contributor

We should move forward; it keeps coming up.

I'd personally prefer if this were maxEventsPerPage or something (reset every page change, as I mentioned). Also, should not call this maxErrors but rather maxEvents since you can also send info or warning level events.

@kamilogorek
Copy link
Contributor

@benvinegar updated the code, docs and TS definitions.

@kamilogorek kamilogorek changed the title can set maxErrors config option can set maxEventsPerPage config option Sep 12, 2017
Copy link
Contributor

@benvinegar benvinegar left a comment

Choose a reason for hiding this comment

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

Biggest issue is that this will only work if breadcrumbs is enabled.

Unfortunately event/DOM instrumentation is tied really closely to breadcrumbs. We need to separate these two concepts.

Sorry! This ended up being tougher than I hinted at 😅

@@ -875,6 +876,9 @@ Raven.prototype = {
var parsedTo = parseUrl(to);
var parsedFrom = parseUrl(from);

// refresh max events count
this._sentEvents = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

If breadcrumbs are disabled, this won't get called / won't reset.

@@ -1255,6 +1259,9 @@ Raven.prototype = {
return function(/* state, title, url */) {
var url = arguments.length > 2 ? arguments[2] : undefined;

// refresh max events count
self._sentEvents = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above.

@kamilogorek
Copy link
Contributor

Ah, right. I didn't realise that.

Also we won't have location changes when we'll unify node/browser, which mean that we'd have to diverge an API once again in this case... I'm not sure about integrating that perPage in this case.
Making it a "total" number of events would be much easier and could be pulled to other SDKs as well easily.

@benvinegar
Copy link
Contributor

benvinegar commented Sep 13, 2017

Truly unifying these APIs is probably impossible. A common core is more realistic. We'll probably need to sit down and document what features are in both and see how we can get there.

Edit: I did think for a bit about how we could make this more generic, e.g. so we could use it server-side as well, but nothing comes to mind. maxEventsPerSession isn't right since a session can persist many pages. I mean, maxEventsPerPage could be used per Express-view.

@kamilogorek kamilogorek mentioned this pull request Sep 28, 2017
@beck
Copy link

beck commented Nov 14, 2017

Using Sentry.io, the overage fees are priced per event, making this feature is essential. Especially if the consuming platform limits the ability to use whitelists, blacklists, and filters to cull all abusive scenarios.

@kamilogorek
Copy link
Contributor

Linked this PR discussion to proper docs

@kamilogorek kamilogorek deleted the max-errors branch January 23, 2018 13:37
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.

5 participants