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

Support middleware injected by AppDynamics #4119

Merged
merged 1 commit into from Feb 5, 2019

Conversation

mschnee
Copy link

@mschnee mschnee commented Jan 21, 2019

Description

Much like the workaround for NewRelic, appd replaces the express handle with its own, and stores it in handle.__appdynamicsProxyInfo__.orig. This causes proto._findLayerByHandler to fail.

This pull request was originally created by @mikewli and @gconaty - I have updated it to include unit tests.

Checklist

  • New tests added or existing tests modified to cover all changes
    • Unit tests have been added
  • Code conforms with the style guide
    • All lint tests passes

@slnode
Copy link

slnode commented Jan 21, 2019

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@mschnee
Copy link
Author

mschnee commented Jan 21, 2019

Note for the softlayer status checks: I do not have permission to grant access to the private repository into which loopback has been forked for this fix.

@jannyHou
Copy link
Contributor

@slnode test please

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

@mschnee Thank you and @mikewli , @gconaty for providing the fix+test. I left a nitpick, the change looks reasonable to me, while I am interested to know the background of the issue, could you post a link or related story here?

@@ -225,7 +225,11 @@ proto._findLayerByHandler = function(handler) {
for (var k = this._router.stack.length - 1; k >= 0; k--) {
if (this._router.stack[k].handle === handler ||
// NewRelic replaces the handle and keeps it as __NR_original
this._router.stack[k].handle['__NR_original'] === handler
this._router.stack[k].handle['__NR_original'] === handler ||
Copy link
Contributor

@jannyHou jannyHou Jan 21, 2019

Choose a reason for hiding this comment

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

@mschnee could you refactor the assertion to be a variable, like

var OrigHasHandler = this._router.stack[k].handle['__appdynamicsProxyInfo__'] &&
        this._router.stack[k].handle['__appdynamicsProxyInfo__']['orig'] === handler
      
var handlerExists = this._router.stack[k].handle === handler || this._router.stack[k].handle['__NR_original'] === handler || OrigHasHandler
if (handlerExists) {
// ...code
}

Copy link
Member

Choose a reason for hiding this comment

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

+1, let's clean up this complicated condition please and split it into multiple smaller conditions with their results stored in well-named variables.

E.g.

const current = this._router.stack[k].handle;

const isOriginal = current === handler;
// NewRelic replaces the handle and keeps it as __NR_original
const isNewRelic = current['__NR_original'] === handler;
 // AppDynamics replaces the handle and keeps it as __appdynamicsProxyInfo__.orig
const isAppDynamics = // ...

if (isOriginal || isNewRelic || isAppDynamics) {
  return this._router.stack[k];
} else {
 // ...
}

Copy link
Author

Choose a reason for hiding this comment

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

I've cleaned up the complicated conditions to be a bit more human readable.

Unfortunately, @jannyHou , I don't have very much context into the background of this issue :(

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

The patch looks mostly good, thank you for the contribution ❤️

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

LGTM 👏

Could you please squash all commits into a single one and provide a descriptive commit message following our Commit Message Guidelines?

In case you are not familiar with git history rewriting:

git rebase -i master
# edit the instructions to squash commits
git push --force-with-lease

I'll leave it up to @jannyHou to do the final approval and landing.

@bajtos bajtos self-assigned this Jan 24, 2019
@bajtos
Copy link
Member

bajtos commented Jan 24, 2019

@mschnee one more thing: before we can accept your contribution, we need you to sign our CLA - see https://cla.strongloop.com/agreements/strongloop/loopback

@mschnee
Copy link
Author

mschnee commented Jan 24, 2019

Thanks for the review! I have squashed the commits and provided a more descriptive commit message.

I have also myself accepted the CLA
image

@bajtos
Copy link
Member

bajtos commented Jan 25, 2019

Thank you, @mschnee. I see that the commit was authored by two people, you and @mikewli. We need all authors to sign our CLA. Could you please talk to @mikewli and get him to sign our CLA too?

Also the commit is message is unfortunately not exactly matching our guidelines, this is the report reported by our Commit Linter check:

**************************************************
**
**  Linting commit logs
**
**  1 problems found:
**    c29bad3 - Add a workaround to avoid conflicts with AppDynamic: First line should be 50 characters or less (saw 53)
**
**************************************************

Could you please fix it? Suggested first line: Support middleware injected by AppDynamics

@bajtos bajtos added the feature label Jan 25, 2019
AppDynamics injects a proxy object into the router stack, which it
uses for its network analysis.  This is similar to how NewRelic
adds a sentinel handler to the router stack. This commit adds a
similar workaround so that loopback can find the original layer.
@mschnee
Copy link
Author

mschnee commented Jan 25, 2019

Commit ammended. I'm attempting to reach out to @mikewli

@bajtos bajtos changed the title Add a workaround to avoid conflicts with AppDynamics Support middleware injected by AppDynamics Jan 28, 2019
@mikewli
Copy link
Contributor

mikewli commented Feb 2, 2019

Approved, thanks for the update!

@bajtos bajtos merged commit c38900b into strongloop:master Feb 5, 2019
@bajtos
Copy link
Member

bajtos commented Feb 5, 2019

Landed, thank you @mschnee & @mikewli for the contribution ❤️

@bajtos
Copy link
Member

bajtos commented Feb 5, 2019

Published in loopback@3.25.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants