Skip to content
This repository has been archived by the owner on Jan 5, 2019. It is now read-only.

More monitor #98

Merged
merged 6 commits into from Jun 30, 2016
Merged

More monitor #98

merged 6 commits into from Jun 30, 2016

Conversation

jonasfj
Copy link
Contributor

@jonasfj jonasfj commented Jun 28, 2016

I'll enable the lint test and fix issues with it later...

@jonasfj jonasfj assigned imbstack and gregarndt and unassigned imbstack Jun 28, 2016
// https://github.com/taskcluster/taskcluster-lib-monitor/pull/27
await this.monitor.reportError(err, 'error', {}, true);
// Crash the process
process.exit(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this behavior should be factored out to somewhere common, perhaps with tests?

Copy link
Collaborator

@gregarndt gregarndt Jun 29, 2016

Choose a reason for hiding this comment

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

+1, especially since this piece I think has bitten us twice now and is used in multiple places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that would be nice... It won't be in this PR though...
Ideally, some generic way of constructing a process that polls from azure queue and operates on the messages...

@gregarndt
Copy link
Collaborator

Took a look and I think the larged chunk of comments is regarding awaiting for the resolution of the reportError() promise.

@jonasfj
Copy link
Contributor Author

jonasfj commented Jun 30, 2016

I have mixed feelings with regards to waiting for the monitor.reportError to resolve...

I see error reporting as a best effort service... So I'm not sure there is any need to wait for it in most cases... I do it if the error is really critical...

But it's mostly for cases where I want to shutdown after reporting the error...

@jonasfj
Copy link
Contributor Author

jonasfj commented Jun 30, 2016

Does that make sense?

@gregarndt
Copy link
Collaborator

makes sense, thanks.

@jonasfj jonasfj merged commit 15ac6de into master Jun 30, 2016
imbstack pushed a commit that referenced this pull request Dec 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants