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

Fix support for play2.6 versions after play2.6.13 #97

Merged
merged 1 commit into from
Oct 30, 2020

Conversation

junder31
Copy link
Contributor

Reopening #94 on a side branch as requested.

Overview

Monitoring on scala/play2.6.18 was not working when I initially installed the newrelic agent. I found that the later versions of play2.6 deprecated the getHandlerFor method on the Server trait and used the Server object similar to how the play2.7 integration works. Since the signatures for the getHandlerFor vary between the trait and object I had to do some refactoring to support both methods.

Testing

I added a new tests for play2.6.18 which uses the object method rather than the trait.

Checks

[ ] Are your contributions backwards compatible with relevant frameworks and APIs? No
[ ] Does your code contain any breaking changes? No
[ ] Does your code introduce any new dependencies? No

@tspring
Copy link
Contributor

tspring commented Oct 29, 2020

Looks like I gave you bad advice about the SkipIfPresent, and both instrumentation modules match for play 2.6.12. I'll poke around and try to find a solution, but wanted to give you a heads up. Thanks again @junder31

@junder31
Copy link
Contributor Author

@tspring I noticed that. The signatures on the methods differ between 2.6.12 and 2.6.13 though. Won't weaver actually stop it if the actual signature doesn't math when the app starts up? I thought that was the behavior I was seeing when I ran with newrelic in debug mode.

@tspring
Copy link
Contributor

tspring commented Oct 30, 2020

It looks like a type erasure issue and the return type is just Either<A,B> in both 2.12/13. Weaving AsciiSet in the Play-2.13 instrumentation like so, should get around the issue:


import com.newrelic.api.agent.weaver.scala.{ScalaMatchType, ScalaWeave}

// This is weaved to prevent from matching on version 2.12
@ScalaWeave(`type` = ScalaMatchType.Object)
class AsciiSet {
}

@junder31
Copy link
Contributor Author

@tspring Thanks for the help with finding the right classes to weave/skipIfPresent. It looks like that did the trick locally for me.

@tspring
Copy link
Contributor

tspring commented Oct 30, 2020

👍 Looks good, I'll merge as soon as the tests pass and it should be in the next release (6.2.0). Thanks again for your work on this, it's awesome

Copy link
Contributor

@jasonjkeller jasonjkeller 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 awesome. Excellent contribution @junder31 ! 🥇

@tspring tspring merged commit e4f0ef7 into newrelic:main Oct 30, 2020
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