Feature/deduplication #58

srijken · 2016-09-27T22:23:53Z

Similar to the work in the .NET client. This makes sure that:

the occurrence count is set in the deduplicated event, instead of just dropping events.
The date is set to the max of the dates of the rolled up events

make sure we have different stacks when the tests call for it make sure we have a stack when needed

JSON.stringify shortcut didn't work, resulting JSON stack was [{}, {}, {}] Fallback to running through inner errors, calculating hashcode per error, XORing them to a final hashcode

When running the node tests, we need NodeErrorParser, not DefaultErrorParser

because the testsuite process contains a lot of modules, we need to allow more time for module collection

niemyjski · 2016-09-27T22:33:43Z

dist/exceptionless.d.ts

    private _processedHashcodes;
    private _getCurrentTime;
-    constructor(getCurrentTime?: () => number);
+    private _interval;
+    constructor(getCurrentTime?: () => number, interval?: number);


should interval have a default value (you can do defaults in typescript)?

I see that it's there, I just wonder if the definition needs to be regenerated.

Not sure if the definition needs to contain the default value?

I'm not sure either.

I guess that it generates it correctly?

niemyjski · 2016-09-27T22:35:03Z

src/plugins/default/DuplicateCheckerPlugin-spec.ts

-    let context: EventPluginContext;
-    let contextData: ContextData;
-    ({ context, contextData } = createFixture());
+    let errorParser = client.config.errorParser;


I guess this allows us to run this test also on node, the con is we have to new up a whole new client.

We also need the other plugins (to do stack parsing etc) Is there a better way to run all the plugins?

Well for simple tests like this, I figured it would be better to just run the plugins that are required so it's faster and you are running only what is needed.

niemyjski · 2016-09-27T22:38:56Z

src/plugins/default/DuplicateCheckerPlugin.ts

 import { IEventPlugin } from '../IEventPlugin';
 import { EventPluginContext } from '../EventPluginContext';
 import { Utils } from '../../Utils';

 export class DuplicateCheckerPlugin implements IEventPlugin {
-  public priority: number = 40;
+  public priority: number = 90;


The priority should be really really high, in .net we do 1010 to ensure it runs last.

niemyjski · 2016-09-27T22:40:39Z

src/plugins/default/DuplicateCheckerPlugin.ts


-  constructor(getCurrentTime: () => number = () => Date.now()) {
+  constructor(getCurrentTime: () => number = () => Date.now(), interval: number = 60000) {


I wonder if it should be less than a minute.. Minute in .net is not that long, minute in the browser is a long time.

Suggestions for a better interval? How do we make sure the event gets sent anyway when the user navigates away?

there might be an event for that? I know there are libs that you can get notifications of when the page goes to sleep or isn't active, maybe we'd turn off queue processing / submit all events when that happens? Idk

I'd go for a 30second interval I think

niemyjski · 2016-09-27T22:41:58Z

src/plugins/default/DuplicateCheckerPlugin.ts

-          log.info(`Ignoring duplicate error event hash: ${hashCode}`);
-          return true;
+      while (error) {
+        if (error.stack_trace && error.stack_trace.length) {


what if there is no stack trace? Should message be taken into account always?

niemyjski · 2016-09-27T22:42:46Z

src/plugins/default/DuplicateCheckerPlugin.ts

-          return true;
+      while (error) {
+        if (error.stack_trace && error.stack_trace.length) {
+          hashCode = (hashCode * 397) ^ Utils.getHashCode(JSON.stringify(error.stack_trace));


should this be hashCode +=?

niemyjski · 2016-09-27T22:44:30Z

src/plugins/default/DuplicateCheckerPlugin.ts

+      return;
+    }
+
+    this._processedHashcodes.push({ hash: hashCode, timestamp: now });


would be nice if we added some minimal logging to this class. Makes it much easier to debug issues

… trace

niemyjski · 2016-09-28T13:42:14Z

src/plugins/default/DuplicateCheckerPlugin.ts

@@ -28,8 +28,10 @@ export class DuplicateCheckerPlugin implements IEventPlugin {
      let hashCode = 0;

      while (error) {
-        if (error.stack_trace && error.stack_trace.length) {
-          hashCode = (hashCode * 397) ^ Utils.getHashCode(JSON.stringify(error.stack_trace));
+        hashCode += (hashCode * 397) ^ Utils.getHashCode(error.message);


Do you think we should be checking the message length as well? Os should getHashCode method check that?

Added it, just in case :)

niemyjski · 2016-09-28T13:42:39Z

src/plugins/default/DuplicateCheckerPlugin.ts

      context.cancelled = true;
      return;
    }

    if (this._processedHashcodes.some(h => h.hash === hashCode && h.timestamp >= (now - this._interval))) {
+      context.log.info("Adding event with hash: " + hashCode);


This should be a trace level message

We only have these:

info(message: string): void; warn(message: string): void; error(message: string): void;

niemyjski · 2016-09-28T13:42:43Z

src/plugins/default/DuplicateCheckerPlugin.ts

      this._mergedEvents.push(new MergedEvent(hashCode, context, count));
      context.cancelled = true;
      return;
    }

+    context.log.info("Enqueueing event with hash: " + hashCode + "to cache.");


This should be a trace level message

We only have:

info(message: string): void; warn(message: string): void; error(message: string): void;

Guess we could add trace (https://github.com/DeveloperToolsWG/console-object/blob/master/api.md#consolelogobject--object-) doesn't look like it's supported everywhere. We'd have to wrap it like we are doing the other ones I guess.

Errors with just a message are also deduplicated now

srijken added 15 commits May 14, 2016 01:57

Start work to make the deduplication plugin work like the .NET one

fd0f3bb

Cleanup and make sure we have stacks

0fc7f0c

make sure we have different stacks when the tests call for it make sure we have a stack when needed

Better hashcode calculation

7c12490

JSON.stringify shortcut didn't work, resulting JSON stack was [{}, {}, {}] Fallback to running through inner errors, calculating hashcode per error, XORing them to a final hashcode

Merge master

c609e4c

Use the configured error parser

1e57e9e

When running the node tests, we need NodeErrorParser, not DefaultErrorParser

Fix lint error

93b29f0

Merge master

8a1ebc3

Files from previous commit; merge from master

0f8261a

Increase timeouts

e41652d

because the testsuite process contains a lot of modules, we need to allow more time for module collection

load default settings / injects in testsuite

b3419f1

use fake XMLHttpRequests

4cfc5fa

Add sinon

fff0f69

Remove unused variable

09cd805

Create a real exception in the reference id test

06d81f2

Get rid of higher timeout values

ef44761

niemyjski reviewed Sep 27, 2016

View reviewed changes

srijken added 5 commits September 28, 2016 13:58

use the same priority as the .NET client

e2968cc

Use += instead of =

1bbe4bf

Also take the message into account, to deal with errors without stack…

2a35c09

… trace

Add logging

f7c9b2d

correct the case where no stack trace is found

b26971a

niemyjski reviewed Sep 28, 2016

View reviewed changes

srijken added 6 commits October 3, 2016 21:21

use 30 secs as deduplication interval

bf1baab

Add check on error.message

be062f7

Fix lint messages

63c94a1

Fix unit test

66f40c3

Errors with just a message are also deduplicated now

Run build

cdf55c8

Change some .info calls to .trace

bb0414d

srijken added 2 commits October 4, 2016 21:36

Merge branch 'master' into feature/deduplication

9651726

ran the build

ef5f477

srijken merged commit c24d435 into master Oct 4, 2016

srijken deleted the feature/deduplication branch October 4, 2016 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/deduplication #58

Feature/deduplication #58

srijken commented Sep 27, 2016 •

edited

Loading

niemyjski Sep 27, 2016

niemyjski Sep 27, 2016

srijken Sep 28, 2016

niemyjski Sep 28, 2016

srijken Oct 4, 2016

niemyjski Sep 27, 2016

srijken Sep 28, 2016

niemyjski Sep 28, 2016

niemyjski Sep 27, 2016

srijken Sep 28, 2016

niemyjski Sep 27, 2016

srijken Sep 28, 2016

niemyjski Sep 28, 2016

niemyjski Sep 28, 2016

srijken Oct 3, 2016

niemyjski Sep 27, 2016 •

edited

Loading

srijken Sep 28, 2016

niemyjski Sep 27, 2016

srijken Sep 28, 2016

niemyjski Sep 27, 2016

srijken Oct 3, 2016

niemyjski Sep 28, 2016

srijken Oct 3, 2016

niemyjski Sep 28, 2016

srijken Oct 3, 2016

niemyjski Sep 28, 2016

srijken Oct 3, 2016

niemyjski Oct 3, 2016

srijken Oct 4, 2016


		constructor(getCurrentTime: () => number = () => Date.now()) {
		constructor(getCurrentTime: () => number = () => Date.now(), interval: number = 60000) {

Feature/deduplication #58

Feature/deduplication #58

Conversation

srijken commented Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemyjski Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srijken commented Sep 27, 2016 •

edited

Loading

niemyjski Sep 27, 2016 •

edited

Loading