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

WIP Release/v0.13.11 #71

Merged
merged 35 commits into from
Mar 22, 2018
Merged

WIP Release/v0.13.11 #71

merged 35 commits into from
Mar 22, 2018

Conversation

michaloo
Copy link
Contributor

tl;dr

PR adds custom Error classes which when thrown or passed to promise rejection will be treated differently - it won't be sent to sentry but a datadog metric connector.transient_error with two tags error_name and error_message will be instrumented to see the rate of specific error and optionally bubble up to an internal alert or customer facing information.

full changelog:

  • this release brings bigger changes to error handling:
    • it cleans up a little middleware stack including smart-notifier errors
    • it introduces two types of errors - unhandled error which is handled the same as till now, and transient error which won't be pushed to sentry, but only instrumented in datadog
    • it deprecates dedicated smartNotifierErrorMiddleware
    • smartNotifierHandler in case of error behaves like notifHandler and pass the error down the middleware stack
  • added timeout option to Hull.Connector constructor to control the timeout value
  • upgrades raven library
  • flow types fixes

@unity
Copy link
Contributor

unity commented Mar 15, 2018

@michaloo can you document usage examples in readme.md

Sent with GitHawk

README.md Outdated
return Promise.resolve();
}
```
Hull Node promotes using [SuperAgent](http://visionmedia.github.io/superagent/) as a core HTTP client. We provide two plugins to add more instrumentation over the requests.
Copy link
Contributor

Choose a reason for hiding this comment

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

@michaloo missing the actual plugins descriptions here

@michaloo
Copy link
Contributor Author

@unity sure, still doing major moves between README.md (focused around how-to and getting started and examples) and API.md (precise and exact information about tools you already know how to use). Thanks for fixes.

API.md Outdated

Returns **[Function][61]**

## Types
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird to have Types in the middle of the rest.

API.md Outdated

Connector (also called ship) object with settings, private settings and manifest.json

Type: {id: [string][58], updated_at: [string][58], created_at: [string][58], name: [string][58], description: [string][58], tags: [Array][64]<[string][58]>, manifest: [Object][57], settings: [Object][57], private_settings: [Object][57], status: [Object][57]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting broken

API.md Outdated
Context added to the express app request by hull-node connector sdk.
Accessible via `req.hull` param.

Type: {requestId: [string][58], config: [Object][57], token: [string][58], client: [Object][57], ship: THullConnector, connector: THullConnector, hostname: [string][58], options: [Object][57], connectorConfig: [Object][57], segments: [Array][64]<THullSegment>, cache: [Object][57], metric: [Object][57], enqueue: [Function][61], helpers: [Object][57], service: [Object][57], shipApp: [Object][57], message: [Object][57]?, notification: [Object][57], smartNotifierResponse: [Object][57]?}
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting broken

API.md Outdated

Returns **[Promise][62]**

## Context
Copy link
Contributor

Choose a reason for hiding this comment

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

missing introductory paragraph with a code snippet of all the possible items? like on https://github.com/hull/hull-node/blob/develop/API.md#hullconnector

API.md Outdated

Returns **http.Server**

## Errors
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to reorder content?
I'd do:

  • HullConnector
  • Hull.Middleware
  • Context
  • Infra
  • Utils
  • Helpers (btw, seems redundant to have Utils & Helpers - but that's probably a refactor)
  • Types
  • Errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good one, I will set that order. Thanks for input.

API.md Outdated

### cache

Cache available as `req.hull.cache` object
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a quick line on Usage and how it relates to CacheAgent would help here.

API.md Outdated

### metric

Metric agent available as `req.hull.metric` object
Copy link
Contributor

Choose a reason for hiding this comment

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

Some simple code sample of usage would help

API.md Outdated

**Properties**

- `requestId` **[string][58]**
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to render it as raw JS ?

I feel here a format such as

type {
  requestId: string
}

would be much more explicit


General utilities

### notifHandler
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to document this since it's deprecated ?

API.md Outdated

### smartNotifierHandler

`smartNotifierHandler` is a next generation `notifHandler` cooperating with our internal notification tool. It handles Backpressure, throttling and retries for you and lets you adapt to any external rate limiting pattern.
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're deprecating notifHandler we don't want to refer to it here I guess. And we can rename internal notification tool to Kraken.

@michaloo michaloo merged commit c50c701 into master Mar 22, 2018
@michaloo michaloo deleted the release/v0.13.11 branch March 22, 2018 16:48
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.

4 participants