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

Update default ajv instance to include all errors #1398

Merged
merged 1 commit into from
Jan 31, 2019
Merged

Conversation

cemremengu
Copy link
Contributor

@cemremengu cemremengu commented Jan 17, 2019

Closes #1397

Set allErrors: true in default AJV instance to catch all errors

Checklist

  • run npm run test
  • run npm run benchmark
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message and code follows Code of conduct

@cemremengu cemremengu requested a review from a team January 17, 2019 13:11
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM, I’m in favor of this change.

@mcollina
Copy link
Member

(only for v2)

Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

Out of curiosity, can you also post a benchmark?

@delvedor delvedor added the semver-major Issue or PR that should land as semver major label Jan 17, 2019
@cemremengu
Copy link
Contributor Author

cemremengu commented Jan 17, 2019

I don't trust myself or my environment with benchmarks so would be great if someone double check this. 😄 There seems to be no change but it also depends on the size and complexity of the schema I guess.

Using sample

'use strict'

const fastify = require('../fastify')()

const schema = {
  body: {
    type: 'object',
    properties: {
      name: { type: 'string' },
      work: { type: 'string' }
    },
    required: ['name', 'work']
  }
}

fastify.post('/', { schema }, function (req, reply) {
  reply.send({ hello: 'world' })
})

fastify.listen(3000)

Before

PS C:\Users\cemre.mengu\Desktop\fastify> npx autocannon -m POST -H "content-type=application/json" -b "{}" -c 100 -d 5 -p 10 localhost:3000
Running 5s test @ http://localhost:3000
100 connections with 10 pipelining factor

┌─────────┬──────┬──────┬───────┬────────┬─────────┬──────────┬──────────┐
│ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%    │ Avg     │ Stdev    │ Max      │
├─────────┼──────┼──────┼───────┼────────┼─────────┼──────────┼──────────┤
│ Latency │ 0 ms │ 0 ms │ 90 ms │ 110 ms │ 8.73 ms │ 26.65 ms │ 237.4 ms │
└─────────┴──────┴──────┴───────┴────────┴─────────┴──────────┴──────────┘
┌───────────┬─────────┬─────────┬─────────┬────────┬────────┬─────────┬─────────┐
│ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%  │ Avg    │ Stdev   │ Min     │
├───────────┼─────────┼─────────┼─────────┼────────┼────────┼─────────┼─────────┤
│ Req/Sec   │ 9031    │ 9031    │ 11839   │ 11903  │ 11236  │ 1112.09 │ 9029    │
├───────────┼─────────┼─────────┼─────────┼────────┼────────┼─────────┼─────────┤
│ Bytes/Sec │ 2.66 MB │ 2.66 MB │ 3.48 MB │ 3.5 MB │ 3.3 MB │ 327 kB  │ 2.65 MB │
└───────────┴─────────┴─────────┴─────────┴────────┴────────┴─────────┴─────────┘

Req/Bytes counts sampled once per second.

0 2xx responses, 56183 non 2xx responses
56k requests in 5.08s, 16.5 MB read

After

PS C:\Users\cemre.mengu\Desktop\fastify> npx autocannon -m POST -H "content-type=application/json" -b "{}" -c 100 -d 5 -p 10 localhost:3000
Running 5s test @ http://localhost:3000
100 connections with 10 pipelining factor

┌─────────┬──────┬──────┬───────┬───────┬────────┬──────────┬───────────┐
│ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%   │ Avg    │ Stdev    │ Max       │
├─────────┼──────┼──────┼───────┼───────┼────────┼──────────┼───────────┤
│ Latency │ 0 ms │ 0 ms │ 86 ms │ 93 ms │ 8.4 ms │ 25.54 ms │ 171.58 ms │
└─────────┴──────┴──────┴───────┴───────┴────────┴──────────┴───────────┘
┌───────────┬─────────┬─────────┬─────────┬─────────┬─────────┬────────┬─────────┐
│ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg     │ Stdev  │ Min     │
├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼────────┼─────────┤
│ Req/Sec   │ 10855   │ 10855   │ 11855   │ 12391   │ 11666.4 │ 652.06 │ 10852   │
├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼────────┼─────────┤
│ Bytes/Sec │ 3.19 MB │ 3.19 MB │ 3.49 MB │ 3.64 MB │ 3.43 MB │ 192 kB │ 3.19 MB │
└───────────┴─────────┴─────────┴─────────┴─────────┴─────────┴────────┴─────────┘

Req/Bytes counts sampled once per second.

0 2xx responses, 58327 non 2xx responses
58k requests in 5.08s, 17.1 MB read

@delvedor
Copy link
Member

You should test a case where you have multiple schema failures :)

@cemremengu
Copy link
Contributor Author

@delvedor I was not kidding when I said you shouldn't trust me 😄

Updated the previous comment

Copy link
Contributor

@nwoltman nwoltman left a comment

Choose a reason for hiding this comment

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

This is a good idea 👍

@mcollina
Copy link
Member

I would prefer to run benchmakrs myself before landing

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM this is not regressing anything

@mcollina mcollina merged commit b5a961e into master Jan 31, 2019
@mcollina mcollina deleted the all-errors branch January 31, 2019 16:03
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
semver-major Issue or PR that should land as semver major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants