reusing socket #10

litzenberger · 2017-08-10T15:32:41Z

No description provided.

mcollina

The changes fo the test should not be needed. However, I would like to have a test. Maybe in the unit ones?

mcollina · 2017-08-11T16:45:27Z

pino-eventhub.js

+                debug("socket has been closed");
+            });
+          }
+        });


why are we taking track of the socket?

I was just using it to show that the socket was being reused. I can remove that.

mcollina · 2017-08-11T16:46:24Z

pino-eventhub.js

@@ -28,7 +30,7 @@ function createSignature (uri, ttl, sapk, warn) {
  return encodeURIComponent(hash)
 }

-function pinoEventHub (opts) {
+function pinoEventHub (opts, keepAliveAgent, sockets) {


I think the signature shoul stay as before.

yep got it.

mcollina · 2017-08-11T16:46:50Z

start.js

@@ -23,6 +25,9 @@ function start (opts) {
  const sapn = opts['shared-access-policy-name'] || process.env.PINO_SHARED_ACCESS_POLICY_NAME
  const sapk = opts['shared-access-policy-key'] || process.env.PINO_SHARED_ACCESS_POLICY_KEY
  const sas = opts['sas'] || process.env.PINO_SHARED_ACCESS_SIGNATURE
+  const agent = new https.Agent({ keepAlive: true ,maxSockets: 1});


I would make the number of sockets configurable, with a default like 10.

mcollina · 2017-08-11T16:47:54Z

pino-eventhub.js

@@ -10,6 +10,8 @@ const https = require('https')
 const debug = require('debug')('pino-eventhub')
 const Parse = require('fast-json-parse')

+
+


these spaces are not needed. Are we running standard here? It should fail. If not, let's add it.

We are running standard. I'll verify this.

coveralls · 2017-08-13T03:26:22Z

Changes Unknown when pulling 185d8dc on http-agent into ** on master**.

coveralls · 2017-08-14T13:41:52Z

Changes Unknown when pulling ff7364f on http-agent into ** on master**.

mcollina · 2017-08-16T15:35:44Z

test/pino-eventhub.js

@@ -39,6 +39,7 @@ lab.experiment('Pino Event Hub', () => {
  lab.test('returns done', (done) => {
    const sig = pino.createSignature(uri, se, sapk, true)
    expect(sig).to.exist()
+    console.log(sig)


can you remove this?

mcollina · 2017-08-16T15:36:11Z

.travis.yml

-  - PINO_SHARED_ACCESS_POLICY_NAME=sendPinoEvent 
-  - secure: NWCKKPmwiUJDzeI0tuG9EyOMZ1M8we2yDeG++Qftwk9pYjsOdT2ln6nZEYYMZbZIgMHHhoSfB/F6+ayNWn8fJU7b+WYma2lZjrNqii1szWKrAKKp4BW2Popm/fbl/7btUCrONsOQxA8x9XN8dK3dI0ocdu88eNFd7eRzJrUlOj+DgOto7bJfCnnDCqmAAohR0nD0FzYIRlxvj+w/ze4I60DU3cYdDraOEgw9VzrJx0iTm2cnDgMY3wNFiO5QTcQ6iND73gN7SnG3u/nOgUnTXnhqwX9b4LINe78sSCoKabeLj2gSw66epXQGyJL3VlELGajdBom46GwdJcBS8/iX7h82RZBlsIrNA+jFKEoW7Wehdguv0DKNHlFG+O5qkM++upEMezjTw5nX4f8CLGd4ffrz5JCen5IjBbxafTmZ7ts85tAlrOG9YunxxgySZKEh4mWONdS7jVVKFVEhDXdzfVzHNs4VQ133lrT2JBN21u3QoAlXBshiHdXnlJ5S+nes4r0qQliqKiCtTGOzxD9+hGj9bnhzi0GaUt0W4WAIOd5Y7rh3BMKaDHppIsrHEzRetT38Vf2YxvoRAbZ8Di5qX54SRVJKj2wE6TDdCDdaceWiKsabRlmVd7qWY6mQDFQ2i96JytOad0AAn1RL77PCKaaXpelBKldiXVPtJAj5wqA=
+  - PINO_SHARED_ACCESS_POLICY_NAME=sendPinoEvent
+  - secure: PMCGYxIRYtap+zPJRIOmBlgd4UoVNlEko0nCZFaCgsQoMBCn8FIcDPOVTG8v/di/lvYBtZgr2T+GLcfDIMazzSG2m+nh1LGbErduMollU+jfnHAYmif3vcQaJIAf15EcAXiovfp+tqiinouDyQ+3OjDtLm/NLvBeQAndSo9Ge9eQx+5jXtw17CgfOiTdBmBMqODSF3ZUadQBY5l2YPGIlWjRD5FbAc/tkETiscUCghWBsHUyWg6dw4WswuPr9gjj82a3IBzAy7mIhkeB8aJErqYIpjcclI2VXql9waAxjz+4Ea1S0aY841UlehiStFMDKVByoBbIWYA0/z5wr0feVJSgcMF/qgF975/p+WSOJFpbbyG6NIsV/Ew7llN926Lg5X8w8kkk9l6FC0MJtRsh98QuC+XRhFvt4SuOl9K7LfIXFMmknXclcmALZVzBg6U2gZOkRZFMBttSJrwR3xaU3WFDz4kxtLz3yt9pk26MXyUO2N8lvupoXjk7VNGfDXdy6BriZVPbGBjwia5+l8MIJwEVbt6LZainV/SLZNCjQmd1KRLSSsFtqXPT3ApUkEZnxZSHBQKTm2CG0x25C2Wxcf52bfHpPkyvn31bBc/i3PPs5IIpN5KiLkhnAQOTvIlPfaIzjblC/yLL4OokavUx7I8yf/vzXxI/fzC5HXQt9wE=


can we set them via the travis UI? It might be easier as they need to be changed often.

I'll look into that.

mcollina · 2017-08-16T15:37:03Z

pino-eventhub.js

@@ -9,6 +9,9 @@ const crypto = require('crypto')
 const https = require('https')
 const debug = require('debug')('pino-eventhub')
 const Parse = require('fast-json-parse')
+const socketCount = 10
+
+const agent = new https.Agent({keepAlive: true, maxSockets: socketCount})


this should not be limited to the single module, it should be created within

pino-eventhub/pino-eventhub.js

Line 34 in ff7364f

function pinoEventHub (opts) {

.

mcollina · 2017-08-16T15:37:43Z

start.js

@@ -23,6 +25,8 @@ function start (opts) {
  const sapn = opts['shared-access-policy-name'] || process.env.PINO_SHARED_ACCESS_POLICY_NAME
  const sapk = opts['shared-access-policy-key'] || process.env.PINO_SHARED_ACCESS_POLICY_KEY
  const sas = opts['sas'] || process.env.PINO_SHARED_ACCESS_SIGNATURE
+  const max = opts['max'] || socketCount
+  const agent = new https.Agent({keepAlive: true, maxSockets: max})


we should not create the agent here, but in the other file.

yes, been thinking about this more and need to do some refactoring. Thanks for the feedback.

coveralls · 2017-08-18T04:54:39Z

Changes Unknown when pulling 25aaa5a on http-agent into ** on master**.

coveralls · 2017-08-18T04:54:39Z

Changes Unknown when pulling 25aaa5a on http-agent into ** on master**.

coveralls · 2017-08-18T04:54:39Z

Changes Unknown when pulling 25aaa5a on http-agent into ** on master**.

coveralls · 2017-08-18T05:04:51Z

Changes Unknown when pulling def7210 on http-agent into ** on master**.

mcollina

LGTM

reusing socket

501df63

litzenberger self-assigned this Aug 10, 2017

litzenberger requested a review from mcollina August 10, 2017 15:32

bug fix

ced7121

mcollina reviewed Aug 11, 2017

View reviewed changes

litzenberger added 3 commits August 12, 2017 21:59

cleanup and lint fix

0f58938

update key

a116299

added lint and coveralls to travis

185d8dc

allow maxSockets to be configured

ff7364f

mcollina reviewed Aug 16, 2017

View reviewed changes

litzenberger added 2 commits August 17, 2017 23:46

refactor

c6dfb89

added lint to test

25aaa5a

moved access key to repository settings in travis

def7210

mcollina approved these changes Aug 18, 2017

View reviewed changes

litzenberger merged commit 06e14ce into master Aug 22, 2017

litzenberger deleted the http-agent branch August 22, 2017 03:40

mcollina mentioned this pull request Aug 22, 2017

Use an HTTP agent #3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reusing socket #10

reusing socket #10

litzenberger commented Aug 10, 2017

mcollina left a comment

mcollina Aug 11, 2017

litzenberger Aug 11, 2017

mcollina Aug 11, 2017

litzenberger Aug 11, 2017

mcollina Aug 11, 2017

mcollina Aug 11, 2017

litzenberger Aug 11, 2017

coveralls commented Aug 13, 2017 •

edited

Loading

coveralls commented Aug 14, 2017 •

edited

Loading

mcollina Aug 16, 2017

mcollina Aug 16, 2017

litzenberger Aug 18, 2017

mcollina Aug 16, 2017

mcollina Aug 16, 2017

litzenberger Aug 16, 2017

coveralls commented Aug 18, 2017

coveralls commented Aug 18, 2017

coveralls commented Aug 18, 2017

coveralls commented Aug 18, 2017 •

edited

Loading

mcollina left a comment

		@@ -10,6 +10,8 @@ const https = require('https')
		const debug = require('debug')('pino-eventhub')
		const Parse = require('fast-json-parse')

reusing socket #10

reusing socket #10

Conversation

litzenberger commented Aug 10, 2017

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 13, 2017 • edited Loading

coveralls commented Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 18, 2017

coveralls commented Aug 18, 2017

coveralls commented Aug 18, 2017

coveralls commented Aug 18, 2017 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

coveralls commented Aug 13, 2017 •

edited

Loading

coveralls commented Aug 14, 2017 •

edited

Loading

coveralls commented Aug 18, 2017 •

edited

Loading