refactor: Updated shim.recordConsume to use shim.record and added ability to invoke an after hook with callback args

[bizob2828]

Description

This PR solves a few issues. shim.recordConsume was wrapping a function and had the same logic around binding promises and callbacks as shim.record does. I updated shim.recordConsume to shim.record a consumer which allowed me to removed the redundant code of binding callbacks and promises to the active segment. The only missing piece there was invoking a messageHandler function which was only used in amqplib to copy parameters to the active segment. I enhanced the wrapping callbacks to pass in a spec and invoke an after function with the arguments passed to the callback. By doing this is provides feature parity of the messageHandler. I also updated the after function to pass in all args as an object which caused all instrumentation to be updated that implemented an after function.

Related Issues

Closes #981

[jsumners-nr]

All of this is looking good to me, but I have a nagging concern about the signature changes. We make RecorderSpec public via https://newrelic.github.io/node-newrelic/RecorderSpec.html. So changing the signature seems like a breaking change to me. Are we expecting the various specs's methods to be internal?

[bizob2828]

headers were never getting used here. I could mimic what's being done in here but we never had tests that asserting DT headers were getting propagated. I would think we should. so maybe i'll cut another story to fix this

[bizob2828]

ok i dug into this and I don't think it's apples to apples in recordSubscribecConsume it creates a transaction for the consumption. Whereas .get is just pulling one message. I can't just add the headers to DT payload as there's only 1 transaction getting created elsewhere and not within .get. So I'll leave this as is

[jsumners-nr]

Should that be noted in an inline comment?

[bizob2828]

I'm not sure because there's no code to say we don't need to propagate headers

[bizob2828]

All of this is looking good to me, but I have a nagging concern about the signature changes. We make RecorderSpec public via https://newrelic.github.io/node-newrelic/RecorderSpec.html. So changing the signature seems like a breaking change to me. Are we expecting the various specs's methods to be internal?

I hear ya. I know we have no data to tell if this is being used. The amount of arguments passed was getting unwieldy and I introduced a new one args that's only present for callbacks. I doubt we have people using these parts of the spec

[jsumners-nr]

I hear ya. I know we have no data to tell if this is being used. The amount of arguments passed was getting unwieldy and I introduced a new one args that's only present for callbacks. I doubt we have people using these parts of the spec

I am completely in favor of object based parameters. As long as we are aware of the possibility of breakage, I am not opposed to merging. I also highly doubt anyone will be affected.

[chrisleekr]

Hi @bizob2828

The change of https://github.com/newrelic/node-newrelic/pull/2207/files#diff-c3b4dadcc3b24d7d31b5192193cdb89f33e7c2e9cb2f4b9ddbf7e50067c9240aR125 broke our custom instruments as the signature changed.

If this change is intended, I think you need to update your doc as well - https://newrelic.github.io/node-newrelic/tutorial-Messaging-Simple.html#pub%2Fsub-pattern

[bizob2828]

@chrisleekr sorry for the troubles. We can update that example but we have actually phased it out in favor of working examples here. However, I do notice this needs updated so I will get it all sorted out.

[chrisleekr]

Hi @bizob2828

Thanks for the prompt response!
I appreciate it 😃