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

Unique Identifiers for POST Requests #98

Merged
merged 14 commits into from
Dec 12, 2016
Merged

Unique Identifiers for POST Requests #98

merged 14 commits into from
Dec 12, 2016

Conversation

03k64
Copy link
Collaborator

@03k64 03k64 commented Dec 7, 2016

Problem

In order to automate soak testing of POST endpoints we need to send some unique identifier as part of the request. For example, when registering a new user for a website, we require the email address used to be unique.

Solution

Alteration of AutoCannon to recognise a specific tag, [<uuid>], within the request body and replace it with a new version 4 UUID. UUID generation is handled by the uuid package. In order to ensure per request rather than per connection unique identifiers, the tag is replaced as part of the Request Iterator rather than Request Builder. The Request Builder is also modified to add 28 bytes per usage of the tag to the Content-Length header to account for the replacement.

Donald Robertson added 4 commits December 6, 2016 19:13
…where a unique identifier is required as part of each request such as when creating new users where email must be unique
…st rather than per connection. Added code to correct content length in request builder
…_from function in order to ensure Node <6 compatibility
@mcollina
Copy link
Owner

mcollina commented Dec 7, 2016

Thanks for your contribution.
Can you swap uuid with http://npm.im/hyperid? The latter is way faster.

I only have one request, and it is that the id-replicating logic should be configurable, not enabled by default.

@03k64
Copy link
Collaborator Author

03k64 commented Dec 7, 2016

No problem, it's been fun to work on.

I have replaced uuid with hyperid and made id-replacement configurable as you asked. I've updated the README to reflect the latter change.

@@ -60,7 +60,8 @@ function requestBuilder (defaults) {
}

if (bodyBuf && bodyBuf.length > 0) {
headers['Content-Length'] = '' + bodyBuf.length
const uuidCount = (bodyBuf.toString().match(/\[<uuid>\]/g) || []).length
headers['Content-Length'] = `${bodyBuf.length + (uuidCount * 28)}`
Copy link
Collaborator

@GlenTiki GlenTiki Dec 7, 2016

Choose a reason for hiding this comment

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

The maths relating to this content-length is wrong, I think.

content-length = bodyBuf.length - ('<uuid>'.length * uuidCount) + (uuidCount * 28)

Because the bodyBuf string will remove the '' tokens to replace them with the uuid's

Also is an uuid not 16 bytes in length?

Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I will absolutely do that, I think in my haste to respond to the earlier comment I forgot to go back over the maths. I'd gotten the earlier figure by comparing the content length with the tag and with the UUID, but I will look into this and add a test as you suggested below. Should have time to work on this tonight or tomorrow.

@GlenTiki
Copy link
Collaborator

GlenTiki commented Dec 7, 2016

If you wouldn't mind, could you add a basic test to validate this works as expected? Thank you for this PR, it looks awesome 👍

@@ -90,6 +90,8 @@ Available options:
Don't render the progress bar. default: false.
-l/--latency
Print all the latency data. default: false.
-I/--idReplacement
Enable replacement of [<uuid>] with a randomly generated unique identifier within the request body. default: false.
Copy link
Owner

Choose a reason for hiding this comment

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

call it <id>, or :id:.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to [<id>]

@@ -1,3 +1,4 @@
const uuid = require('hyperid')()
Copy link
Owner

Choose a reason for hiding this comment

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

this is not a uuid.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to hyperid

@@ -28,7 +29,7 @@ RequestIterator.prototype.nextRequestBuffer = function () {

RequestIterator.prototype.move = function () {
// get the current buffer and proceed to next request
const ret = this.currentRequest.requestBuffer
const ret = new Buffer(this.currentRequest.requestBuffer.toString().replace(/\[<uuid>\]/g, uuid()))
Copy link
Owner

Choose a reason for hiding this comment

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

this is where we lose speed. Can we do this change using a function passed in as an option? In this way we could support more scenarios that just :id:.

const ret = this.currentRequest.requestBuffer
const ret = this.reqDefaults.idReplacement
? new Buffer(this.currentRequest.requestBuffer.toString().replace(/\[<uuid>\]/g, uuid()))
: this.currentRequest.requestBuffer
Copy link
Owner

Choose a reason for hiding this comment

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

can we change this logic so that this mangling logic is an opt-in function?

const contentLengthRegex = /\[<contentLength>\]/
const reqStr = ret.toString().replace(/\[<id>\]/g, hyperid())
const bodyLength = reqStr.split(contentLengthRegex)[1].trim().length
ret = new Buffer(reqStr.replace(contentLengthRegex, `${bodyLength}`))
Copy link
Owner

Choose a reason for hiding this comment

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

All those steps are going to be really slow, and we are running it twice.
Isn't there a chance to have this logic into the requestBuilder?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only problem with moving it into requestBuilder is that the length of the replacement value on line 37 will vary. As you will know, hyperid generates IDs made up of a base64 string plus an integer, the integer will grow in length as it gets larger, which prevents setting content length until all replacements have been carried out.

This would not be a problem if we used UUIDs as they would be a fixed size, of course then there is a penalty for constantly regenerating UUIDs. I could fork and submit a change to the implementation of hyperid to pad the iteration section out to a fixed width but I don't know how expensive that would be in terms of the performance of hyperid. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

let's try adding a pad to hyperid. It does not grow indefinitely, but up to https://github.com/mcollina/hyperid/blob/master/hyperid.js#L4. Maybe as an option when we create the hyperid instance.

@mcollina
Copy link
Owner

mcollina commented Dec 7, 2016

Good progress! I think we are nearly there!

@thekemkid what do you think?

@GlenTiki
Copy link
Collaborator

GlenTiki commented Dec 8, 2016

I'm loving this work 👍This feature will be fairly complex to understand purely based on the docs, would an example be appropriate?

Other than that, I don't have much to add, and I don't want to step on your toes @darArch 😄 So for now I'm just keeping an eye on how this pr is progressing, but if you need any help, I'm happy to jump in and give you the support you need where I can! :)

@GlenTiki
Copy link
Collaborator

GlenTiki commented Dec 8, 2016

Also, make sure you add your name to the contributors section in the package.json if that's your thing :)

… back to request builder and simplified request iterator. Updated tests as appropriate and tested manually with personal site. Added name to contributors section :)
Copy link
Owner

@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.

Amazing. LGTM

@mcollina
Copy link
Owner

mcollina commented Dec 9, 2016

@darArch would you like to help maintaining/developing autocannon?

@03k64
Copy link
Collaborator Author

03k64 commented Dec 9, 2016

@mcollina I would definitely be interested. It's been great fun to use and work with/on.

@03k64
Copy link
Collaborator Author

03k64 commented Dec 9, 2016

@mcollina @thekemkid The Travis CI build failed when Node v6 failed to install with NVM, would you be able to retrigger the build?

@03k64
Copy link
Collaborator Author

03k64 commented Dec 9, 2016

@thekemkid Should I add an example body before and after tag replacement to the README?

@mcollina
Copy link
Owner

mcollina commented Dec 9, 2016

@darArch an example will be better, I would like to keep the README thin.

@GlenTiki
Copy link
Collaborator

GlenTiki commented Dec 9, 2016

🎉Great to hear you're happy to join 🎉

I'm rebuilding now, and defer to @mcollina on the example 👍

@mcollina
Copy link
Owner

mcollina commented Dec 9, 2016

@darArch just invited you to the repo!

…dReplacement to default options in run.js, updated README
@03k64
Copy link
Collaborator Author

03k64 commented Dec 9, 2016

Not sure what's going on with the Travis CI builds, but I just added an example in samples/using-id-replacement and linked to it in the README.

@GlenTiki
Copy link
Collaborator

GlenTiki commented Dec 9, 2016

NVM on travis seems to be having some trouble right now, lets leave it be for the evening, and retrigger the build in the morning. If its something deeper, I'll investigate further over the weekend.

@03k64
Copy link
Collaborator Author

03k64 commented Dec 12, 2016

I retriggered the Travis build which has now passed. I don't see any option to retrigger the Appveyor build though, @mcollina would you be able to?

@mcollina
Copy link
Owner

All is good now. @thekemkid would you mind doing a release? (Also check if native-hdr-histogram is all good).

@GlenTiki GlenTiki merged commit 34e23d6 into mcollina:master Dec 12, 2016
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.

3 participants