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

CatchAll method + test on transport #3

Merged
merged 11 commits into from
Mar 9, 2018
Merged

CatchAll method + test on transport #3

merged 11 commits into from
Mar 9, 2018

Conversation

wallet77
Copy link
Contributor

@wallet77 wallet77 commented Mar 8, 2018

No description provided.

@wallet77 wallet77 requested a review from vmarchaud March 8, 2018 15:45
return this.transport.send(err)
}

return null
}

catchAll (opts?: any): Boolean | void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure that's now enabled by default, we should automatically call it inside the init of the feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

src/native.ts Outdated
@@ -0,0 +1,5 @@
interface Error {
prepareStackTrace?: () => void,
Copy link
Contributor

Choose a reason for hiding this comment

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

i think the typings of node are available with @types/node

}

if (listener === 'unhandledRejection') {
error = 'You have triggered an unhandledRejection, you may have forgotten to catch a Promise rejection:\n' + error
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm pretty sure we do not need to modify the error, maybe just add a console.error to indicate that the error come from an unhandledRejection should be enough ?

export default class ErrorCallSites {

static init () {
const orig = Error.prepareStackTrace
Copy link
Contributor

Choose a reason for hiding this comment

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

we should remove this, it has been tested but we saw a lot of performances issues in the past, specially with bluebird that create a lot of error just to keep track of the stack when using polyfill for promise.

@@ -4,7 +4,7 @@ import * as stringify from 'json-stringify-safe'
debug('axm:transport')

export default class Transport {
send (args: Error, print: Boolean): void {
send (args: Error | any, print?: Boolean): number {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe transport should in the manager folder since it need to be started before each feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return this.transport.send(err)
}

return null
}

catchAll (opts?: any): Boolean | void {
Callsites.init()
Copy link
Contributor

Choose a reason for hiding this comment

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

cf message in the callsite file

@vmarchaud
Copy link
Contributor

A little fix should be to check that the error that is received by pmx.notify is actually an error object, a lot of people just send random array etc.

import { Feature } from './featureTypes'
import Meter from '../probes/meter'

export default class ProbeFeature implements Feature {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that we don't want to name it Probe, that was the old name, it's way easier to our customer to understand metrics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -0,0 +1,17 @@
import { Feature } from './featureTypes'
import Meter from '../probes/meter'
Copy link
Contributor

@vmarchaud vmarchaud Mar 8, 2018

Choose a reason for hiding this comment

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

We may want to use a 3rd party package to handle the actual compute, to discuss

@wallet77 wallet77 merged commit 9b9625b into master Mar 9, 2018
@wallet77 wallet77 deleted the setup_structure branch March 9, 2018 09:26
@wallet77 wallet77 restored the setup_structure branch March 9, 2018 09:55
@wallet77 wallet77 deleted the setup_structure branch March 9, 2018 09:57
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