newrelic / newrelic-java-agent

The New Relic Java agent
Apache License 2.0
202 stars 143 forks source link

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

Closed smiklos closed 1 year ago

smiklos commented 2 years ago

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 https://github.com/newrelic/newrelic-java-agent/pull/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)

kford-newrelic commented 2 years ago

@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!

workato-integration[bot] commented 2 years ago

https://issues.newrelic.com/browse/NEWRELIC-4097

smiklos commented 1 year ago

@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....

kford-newrelic commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

workato-integration[bot] commented 1 year ago

Work has been completed on this issue.

smiklos commented 1 year ago

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

kford-newrelic commented 1 year ago

1103

smiklos commented 1 year ago

@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...