Http4s transaction changes #1006

richard-gibson · 2022-09-09T21:45:25Z

Before contributing, please read our contributing guidelines and code of conduct.

Overview

This PR adds support for HTTP4s ember and blaze v0.23 client and server libs. It also fixes a bug where transaction activity was not being cleared from the request thread causing incorrect transaction metrics.

Related Github Issue

Include a link to the related GitHub issue, if applicable

Testing

The agent includes a suite of tests which should be used to
verify your changes don't break existing functionality. These tests will run with
Github Actions when a pull request is made. More details on running the tests locally can be found
here,

Checks

[N] Are your contributions backwards compatible with relevant frameworks and APIs? Removal of support transaction support for cats effect parTraverse was necessary to ensure Transaction Activity could be clear from threads after work.
Cats effect async features still have transaction support.
[N] Does your code contain any breaking changes? Please describe.
[Y] Does your code introduce any new dependencies? Please describe.
New instrumentation for HTTP4 blaze and ember v0.23 libs

meiao · 2022-09-22T14:40:06Z

instrumentation/cats-effect-2/src/main/scala/cats/effect/ec/TokenAwareExecutionContext.scala

+  override def execute(runnable: Runnable): Unit = {
+    delegate.execute(new TokenAwareRunnable(runnable))
+  }
+  override def reportFailure(cause: Throwable): Unit = delegate.reportFailure(cause)


Is this needed?

Yes, I think so, we don't want to lose any custom reportFailure that could be specified by a user in the delegate Execution context

Nevermind, I was really tired at this point and missed that there is a TokenAwareRunnable in there.

meiao · 2022-09-22T21:56:23Z

instrumentation/cats-effect-2/src/main/scala/cats/effect/ec/TokenAwareRunnable.scala

+        AgentBridge.getAgent.getLogger.log(Level.FINEST, s"[IOTick] Txn ${txn.hashCode()} cleared from Thread: ${Thread
+          .currentThread().getName}")
+        AgentBridge.getAgent.getLogger.log(Level.FINEST,
+          s"[IOTick] Txn ${txn.hashCode()} ${if(!txnCleared) "not"} cleared from Thread: ${Thread


I think only the 2nd log statement should be here. Or I am missing something.

meiao · 2022-09-22T22:04:54Z

instrumentation/cats-effect-2/src/main/scala/cats/effect/ec/TokenAwareRunnable.scala

+
+  def getTransaction(tokenAndRefCount: AgentBridge.TokenAndRefCount): Transaction =
+    if (tokenAndRefCount != null && tokenAndRefCount.token != null) tokenAndRefCount.token.getTransaction.asInstanceOf[Transaction]
+  else null


Is this the usual alignment for an else statement like this?
it looks odd being in the same level as the function definition, while the beginning of the statement is one indentation level ahead.

meiao · 2022-09-22T22:09:37Z

instrumentation/cats-effect-2/src/main/scala/cats/effect/ec/TokenAwareRunnable.scala

@@ -0,0 +1,79 @@
+package cats.effect.ec


this class should be in the newrelic namespace

meiao · 2022-09-22T22:10:06Z

instrumentation/cats-effect-2/src/main/scala/cats/effect/ec/TokenAwareExecutionContext.scala

@@ -0,0 +1,15 @@
+package cats.effect.ec


This class should be in the newrelic namespace.

meiao · 2022-09-23T19:59:47Z

instrumentation/cats-effect-3.2/build.gradle

@@ -10,12 +10,12 @@ dependencies {
    implementation("org.typelevel:cats-effect_2.13:3.2.8")


This file was moved. But I think it was meant to be copied.
Same with the next 3 files.

meiao · 2022-09-23T20:39:42Z

instrumentation/cats-effect-3.3.0/src/main/java/com/nr/agent/instrumentation/Utils.java

-      logTokenInfo(tokenAndRefCount, "setting token to thread");
-      AgentBridge.activeToken.set(tokenAndRefCount);
-      tokenAndRefCount.token.link();
+  public static void setThreadTokenAndRefCount(AtomicReference<AgentBridge.TokenAndRefCount> tokenAndRefCount, IOFiber<?> ioFiber) {


Any reason for this method receiving a AtomicReference while all others receive the TokenAndRefCount directly?

instrumentation/cats-effect-3.3.0/src/main/scala/cats/effect/IOFiber_Instrumentation.scala

meiao · 2022-09-23T21:10:19Z

instrumentation/cats-effect-3.3.0/src/main/scala/cats/effect/IOFiber_Instrumentation.scala

+      if (fiberTokenEmpty && threadTokenAndRefCount != null) {
+        if (threadTokenAndRefCount != null) {
+          this.tokenAndRefCount.set(threadTokenAndRefCount)
+        }


the 2nd if statement is redundant

meiao · 2022-09-23T21:26:52Z

instrumentation/cats-effect-3.3.0/src/main/scala/cats/effect/IOFiber_Instrumentation.scala

+        if (threadTokenAndRefCount != null) {
+          this.tokenAndRefCount.set(threadTokenAndRefCount)
+        }
+        incrementTokenRefCount(this.tokenAndRefCount.get())


Is the Weaver.callOriginal() running async and the decrement in done the reverse of this increment?

There is a done method that is the last step to be called in IOFiber in both happy and failure paths. It is instrumented to decrement the token

meiao · 2022-09-26T14:39:32Z

...rver-2.12_0.23/src/main/java/org/http4s/server/blaze/BlazeServerBuilder_Instrumentation.java

@@ -0,0 +1,24 @@
+package org.http4s.server.blaze;


Can you change the package, to avoid confusion.

meiao · 2022-09-26T14:42:50Z

...ze-server-2.12_0.23/src/main/scala/com/nr/instrumentation/http4s/TransactionMiddleware.scala

+  def genHttpApp[F[_] : Sync](httpApp: Kleisli[F, Request[F], Response[F]]): Kleisli[F, Request[F], Response[F]] =
+    Kleisli { req: Request[F] => nrRequestResponse(req, httpApp) }
+
+  def nrRequestResponse[F[_] : Sync](request: Request[F], httpAp: Kleisli[F, Request[F], Response[F]]): F[Response[F]] =


parameter httpApp is missing a p

meiao · 2022-09-26T15:18:00Z

...ze-server-2.12_0.22/src/main/scala/com/nr/instrumentation/http4s/TransactionMiddleware.scala

+      Sync[F].raiseError(throwable)
+    })
+
+  private def expireTokenIfNeceessary(token: Token): Unit =


extra e on Necessary

meiao · 2022-09-26T20:00:19Z

...r-client-2.13_0.23/src/main/scala/com/nr/instrumentation/http4s/InboundResponseWrapper.scala

+    response.headers.headers.find(_.name == CIString(name)).map(_.value).orNull
+
+  override def getHeaders(name: String): util.List[String] =
+    response.headers.headers.map(_.name.toString).asJava


Is this filtering by the name?

meiao · 2022-09-26T20:58:42Z

...r-client-2.12_0.23/src/main/scala/com/nr/instrumentation/http4s/InboundResponseWrapper.scala

+    response.headers.headers.find(_.name == CIString(name)).map(_.value).orNull
+
+  override def getHeaders(name: String): util.List[String] =
+    response.headers.headers.map(_.name.toString).asJava


Is this filtering by name?

meiao · 2022-09-26T21:27:10Z

...client-2.13_0.23/src/main/scala/com/nr/instrumentation/http4s/NewrelicClientMiddleware.scala

+        seg <- Resource.eval(
+          construct {
+          val txn = AgentBridge.getAgent.getTransaction
+          logTokenInfo(AgentBridge.activeToken.get, s"client call for txn $txn")
+          val segment = txn.startSegment("HTTP4S client call")
+          segment.addOutboundRequestHeaders(new OutboundRequestWrapper(req))
+          segment
+        })
+        response <- client.run(req)


Won't these run async?

meiao · 2022-09-26T21:36:04Z

...rver-2.12_0.23/src/main/java/org/http4s/server/blaze/EmberServerBuilder_Instrumentation.java

@@ -0,0 +1,21 @@
+package org.http4s.server.blaze;


Wrong package

meiao · 2022-09-26T21:50:35Z

settings.gradle

@@ -276,7 +286,8 @@ include 'instrumentation:scala-2.9.3'
 include 'instrumentation:scala-2.12.0'
 include 'instrumentation:scala-2.13.0'
 include 'instrumentation:cats-effect-2'
-include 'instrumentation:cats-effect-3'
+include 'instrumentation:cats-effect-3.2'


Probably already fixed, but cats-effect-3 is missing.

richard-gibson requested review from meiao, twcrone, tbradellis and obenkenobi September 9, 2022 21:45

richard-gibson force-pushed the HTTP4S-transaction-changes branch from ad74b94 to 2fc21ff Compare September 12, 2022 10:51

richard-gibson added 2 commits September 13, 2022 19:55

cats effect 2 clear txn state from thread before switch

d8ffae1

cats effect 3 clear transaction state from thread before switch

9256eca

richard-gibson force-pushed the HTTP4S-transaction-changes branch 7 times, most recently from 2bb3ca4 to 506db31 Compare September 19, 2022 22:38

http4s 0.23 ember and blaze support

8be4f9b

richard-gibson force-pushed the HTTP4S-transaction-changes branch from 506db31 to 8be4f9b Compare September 19, 2022 22:51

richard-gibson changed the title ~~Http4 s transaction changes~~ Http4s transaction changes Sep 20, 2022

meiao requested changes Sep 23, 2022

View reviewed changes

richard-gibson added 2 commits September 26, 2022 21:44

move cats-effect-3 to 3.2

51b2e2b

move utility classes to nr namespaces

e33d32b

meiao reviewed Sep 26, 2022

View reviewed changes

http4s package name tidy up

3a4ed1c

meiao approved these changes Sep 27, 2022

View reviewed changes

meiao merged commit 639e832 into main Sep 27, 2022

meiao deleted the HTTP4S-transaction-changes branch September 27, 2022 15:43

kford-newrelic mentioned this pull request Oct 3, 2022

Add new instrumentation module to support http4s-blaze-client_2.12 and http4s-blaze-client_2.13 versions 0.23.n+ #429

Closed

slovvik mentioned this pull request Jan 30, 2023

Performance issues with update 7.11 #1135

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Http4s transaction changes #1006

Http4s transaction changes #1006

richard-gibson commented Sep 9, 2022 •

edited

Loading

meiao Sep 22, 2022

richard-gibson Sep 26, 2022

meiao Sep 26, 2022

meiao Sep 22, 2022

meiao Sep 22, 2022

meiao Sep 22, 2022

meiao Sep 22, 2022

meiao Sep 23, 2022

meiao Sep 23, 2022

meiao Sep 23, 2022

meiao Sep 23, 2022

richard-gibson Sep 26, 2022

meiao Sep 26, 2022

meiao Sep 26, 2022

meiao Sep 26, 2022

meiao Sep 26, 2022

meiao Sep 26, 2022

meiao Sep 26, 2022

meiao Sep 26, 2022

meiao Sep 26, 2022

		@@ -10,12 +10,12 @@ dependencies {
		implementation("org.typelevel:cats-effect_2.13:3.2.8")

Http4s transaction changes #1006

Http4s transaction changes #1006

Conversation

richard-gibson commented Sep 9, 2022 • edited Loading

Overview

Related Github Issue

Testing

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richard-gibson commented Sep 9, 2022 •

edited

Loading