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

Slick + Datasource instrumentation is broken in 7.9.0 with scala 2.13 #1000

Closed
smiklos opened this issue Sep 5, 2022 · 10 comments
Closed

Slick + Datasource instrumentation is broken in 7.9.0 with scala 2.13 #1000

smiklos opened this issue Sep 5, 2022 · 10 comments
Labels
enhancement New feature or request on-roadmap Issue has been added to our product roadmap and will be worked in the coming quarter

Comments

@smiklos
Copy link
Contributor

smiklos commented Sep 5, 2022

Description

We are using Akka http 10.2.+ and Sangria so the only route we monitor is /graphql

After upgrading to the current latest agent version (7.9.0) the agent seems to loose track of transactions across method calls.
I can still manually create a transaction and link to it via its token but the transaction propagation between methods and future chaining no longer works at all. I can only suspect the work related to #876 as cause

Expected Behavior

When a web transaction starts we used to have segments span across the whole app, measuring db time and external calls. Now all of that is gone. Transactions still start but none of the above mentioned segments/metrics are measured anymore.

Your Environment

Tried both openjdk 11 and graalvm 22.1

(Migrate to Jira)

@smiklos smiklos added the bug Something isn't working as designed/intended label Sep 5, 2022
@kford-newrelic
Copy link
Contributor

@smiklos does the behavior you mention happen with Akka HTTP alone or is the combined use of Sangria necessary for the problem to occur? Any chance you have a simple app that can reproduce the problem?

While we can't commit to a date when we can investigate this further, having a small app that repros the problem is helpful!

@kford-newrelic kford-newrelic removed the bug Something isn't working as designed/intended label Sep 30, 2022
@workato-integration
Copy link

@smiklos
Copy link
Contributor Author

smiklos commented Nov 9, 2022

@kford-newrelic I tried to reproduce this locally but it does work using akka http and sangria alone.
However it seems that if I check out the latest main branch of the agent, remove code that was introduced by this pr and build it locally then it actually works totally fine.

Somehow the changes in that pr nukes jdbc datasource (and maybe other) instrumentation.
The pr was meant to fix an issue with scala 2.12 so curious how it affected us running on scala 2.13 but it certainly did.

Essentially commenting out this from ScalaTraitMatcher fixes the issue

        if(isScalaSource && this.interfaces.size() > 0 && ((access & Opcodes.ACC_FINAL) == Opcodes.ACC_FINAL)) {
          context.addScalaFinalField(name);
        }

Update

Adding this exclusion to the ScalaTraitFinalFieldTransformer.doTransform method fixes the issue.

    if(className.equals("slick/util/NewRelicRunnable")) {
      return null;
    }

It looks like when NewRelicRunnable gets reinstrumented it goes through this newly introduced scala final trait transformer and that causes issues.

I tried removing the @trace annotation from the run method of NewRelicRunnable , doing that makes this class not instrumented anymore but than tracing still doesn't work (since I guess the agent doesn't know that it should trace this method too).

This final field transformer should not even pick up this class but it looks like it will transform fields even if they are not trait fields....

@smiklos smiklos changed the title Scala 2.13.8 on Java 11+ and Agent V 7.9.0 no longer links transactions across methods Slick + Datasource instrumentation is broken in 7.9.0 with scala 2.13 Nov 11, 2022
@kford-newrelic
Copy link
Contributor

Scala traits...sigh.

@smiklos thank you for this excellent analysis! We'll review this to see if we can easily build on your work for our next (Jan-Mar) quarter but we cannot commit that at the moment. Stay tuned.

In the meantime, are you able to continue using your own agent build as a workaround?

@smiklos
Copy link
Contributor Author

smiklos commented Nov 30, 2022

@kford-newrelic I guess we can use our own build but I was thinking that perhaps a quick fix would be acceptable by you.

If we marked runnable as val runnable in slick.util.NewRelicRunnable the ScalaTraitFinalFieldTransformer would no longer pick up this class and instrumentation would work again for slick. If that's acceptable I'll make a pr

@kford-newrelic
Copy link
Contributor

@smiklos yes a PR would be awesome - thank you!

@kford-newrelic kford-newrelic added Q4 SC enhancement New feature or request on-roadmap Issue has been added to our product roadmap and will be worked in the coming quarter labels Dec 14, 2022
@workato-integration
Copy link

Work has been completed on this issue.

@smiklos
Copy link
Contributor Author

smiklos commented Feb 13, 2023

@kford-newrelic can you link to the work that has been done to solve this issue?

@kford-newrelic
Copy link
Contributor

#1103

@smiklos
Copy link
Contributor Author

smiklos commented Mar 8, 2023

@kford-newrelic that was just a quick fix but the real problem has not been fixed, the one I outlined very detailed here. Other instrumentations libraries are probably also affected by this issue...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request on-roadmap Issue has been added to our product roadmap and will be worked in the coming quarter
Projects
Archived in project
Development

No branches or pull requests

2 participants