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

+ kamon-armeria module #854

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
8436fb0
+ kamon-armeria module
lucasamoroso Sep 1, 2020
aa08c74
rename p to channelPipeline
lucasamoroso Sep 28, 2020
5dde833
remove semicolons
lucasamoroso Sep 28, 2020
2e45248
fix NPE when Armeria makes a redirect
lucasamoroso Oct 14, 2020
b395138
kamon-armeria reimplementation
lucasamoroso Oct 22, 2020
a2e880a
Operation name refactor and https tests
lucasamoroso Nov 1, 2020
4a8dee2
Add gRPC tests
lucasamoroso Nov 1, 2020
8d51bf9
Fix Kamon context propagation
lucasamoroso Nov 1, 2020
3b5d587
Fix unwrap()
lucasamoroso Nov 2, 2020
8b77a59
refactor route not found
lucasamoroso Nov 14, 2020
5e34824
refactor kamon operation name
lucasamoroso Nov 14, 2020
c3b0a31
refactor armeria server instrumentation
lucasamoroso Nov 15, 2020
ecf6990
rename variables in tests
lucasamoroso Nov 15, 2020
d73aa92
update armeria dependency
lucasamoroso Nov 15, 2020
531698e
rename http instrumentation component
lucasamoroso Nov 22, 2020
80feea6
rename package
lucasamoroso Nov 22, 2020
96cc3fd
update armeria version
lucasamoroso Sep 29, 2021
6493f74
update test routes
lucasamoroso Sep 29, 2021
fb71f65
add blocking route test
lucasamoroso Sep 29, 2021
32acb5b
remove runWithContext call
lucasamoroso Sep 29, 2021
3641ad7
fix tests
lucasamoroso Feb 17, 2022
e2a1b04
apply new sbt syntax
lucasamoroso Feb 17, 2022
3b9e856
kamon-armeria can be built from scala 2.11
lucasamoroso Feb 17, 2022
3fd0f26
fix some warnings
lucasamoroso Feb 17, 2022
54a714b
access armeria ports through reflection instead of bridge
lucasamoroso Feb 25, 2022
bfe41d8
add armeria module description
lucasamoroso Feb 25, 2022
8a34a3d
upgrade armeria version
lucasamoroso Apr 11, 2022
3cf5be7
remove scalapb compiler plugin
lucasamoroso Apr 11, 2022
c5c1571
cleanup context propagation
lucasamoroso Sep 18, 2022
7f74090
move configs to improve reconfiguring Kamon
lucasamoroso Sep 18, 2022
8d041ab
move configs to improve reconfiguring Kamon
lucasamoroso Sep 18, 2022
901bb25
cleanup
lucasamoroso Sep 18, 2022
241f2d2
bump up armeria version
lucasamoroso Oct 10, 2022
ee37133
use route pattern string as server side operation name
lucasamoroso Oct 10, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 41 additions & 3 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,8 @@ val instrumentationProjects = Seq[ProjectReference](
`kamon-lagom`,
`kamon-finagle`,
`kamon-aws-sdk`,
`kamon-alpakka-kafka`
`kamon-alpakka-kafka`,
`kamon-armeria`
)

lazy val instrumentation = (project in file("instrumentation"))
Expand Down Expand Up @@ -656,6 +657,41 @@ lazy val `kamon-alpakka-kafka` = (project in file("instrumentation/kamon-alpakka
)
).dependsOn(`kamon-core`, `kamon-akka`, `kamon-testkit` % "test")

lazy val `kamon-armeria` = (project in file("instrumentation/kamon-armeria"))
.disablePlugins(AssemblyPlugin)
.enablePlugins(JavaAgent)
.settings(instrumentationSettings)
.settings(
libraryDependencies ++= {
val grpcDependencies = {
CrossVersion.partialVersion(scalaVersion.value) match {
case Some((2, n)) if n >= 12 =>
Seq(
"com.linecorp.armeria" % "armeria-grpc" % "1.20.0" % "test",
"com.thesamet.scalapb" %% "scalapb-runtime-grpc" % scalapb.compiler.Version.scalapbVersion % "test"
)
case _ => Nil // ignore this dependencies on scala 2.11 test because test will be ignored and this dependency scalapb-runtime-grpc is no available anymore
}
}
Seq(
kanelaAgent % "provided",
"com.linecorp.armeria" % "armeria" % "1.20.0" % "provided",

scalatest % "test",
logbackClassic % "test"
) ++ grpcDependencies
},
Test / skip := {
CrossVersion.partialVersion(scalaVersion.value) match {
case Some((2, n)) if n >= 12 => false
case _ => true // skip scala 2.11 tests because of compatibility issues with protobuf
}
},
Test / PB.protoSources := Seq(file("instrumentation/kamon-armeria/src/test/protobuf")),
Test / PB.targets := Seq(scalapb.gen() -> (Test / sourceManaged).value)
)
.dependsOn(`kamon-instrumentation-common`, `kamon-testkit` % "test")

/**
* Reporters
*/
Expand Down Expand Up @@ -931,7 +967,8 @@ lazy val `kamon-bundle-dependencies-all` = (project in file("bundle/kamon-bundle
`kamon-okhttp`,
`kamon-caffeine`,
`kamon-lagom`,
`kamon-aws-sdk`
`kamon-aws-sdk`,
`kamon-armeria`
)

/**
Expand All @@ -954,7 +991,8 @@ lazy val `kamon-bundle-dependencies-2-12-and-up` = (project in file("bundle/kamo
`kamon-cats-io-3`,
`kamon-finagle`,
`kamon-tapir`,
`kamon-alpakka-kafka`
`kamon-alpakka-kafka`,
`kamon-armeria`
)

lazy val `kamon-bundle` = (project in file("bundle/kamon-bundle"))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/* =========================================================================================
* Copyright © 2013-2020 the kamon project <http://kamon.io/>
*
* Licensed under the Apache License, Version 2.0 (the "License") you may not use this file
* except in compliance with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the
* License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND,
* either express or implied. See the License for the specific language governing permissions
* and limitations under the License.
* =========================================================================================
*/

package kamon.instrumentation.armeria.client;

import com.linecorp.armeria.client.ClientRequestContext;
import com.linecorp.armeria.client.HttpClient;
import com.linecorp.armeria.client.SimpleDecoratingHttpClient;
import com.linecorp.armeria.common.HttpRequest;
import com.linecorp.armeria.common.HttpResponse;
import kamon.Kamon;
import kamon.instrumentation.armeria.client.timing.Timing;
import kamon.instrumentation.armeria.converters.KamonArmeriaMessageConverter;
import kamon.instrumentation.http.HttpClientInstrumentation;

public class ArmeriaHttpClientDecorator extends SimpleDecoratingHttpClient {
private final HttpClientInstrumentation clientInstrumentation;

protected ArmeriaHttpClientDecorator(HttpClient delegate, HttpClientInstrumentation clientInstrumentation) {
super(delegate);
this.clientInstrumentation = clientInstrumentation;
}

@Override
public HttpResponse execute(ClientRequestContext ctx, HttpRequest req) throws Exception {

final HttpClientInstrumentation.RequestHandler<HttpRequest> requestHandler =
clientInstrumentation.createHandler(KamonArmeriaMessageConverter.getRequestBuilder(req), Kamon.currentContext());

ctx.log()
.whenComplete()
.thenAccept(log -> {
Timing.takeTimings(log, requestHandler.span());
requestHandler.processResponse(KamonArmeriaMessageConverter.toKamonResponse(log));
});

try {
return unwrap().execute(ctx, requestHandler.request());
} catch (Exception exception) {
requestHandler.span().fail(exception.getMessage(), exception).finish();
throw exception;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/* =========================================================================================
* Copyright © 2013-2020 the kamon project <http://kamon.io/>
*
* Licensed under the Apache License, Version 2.0 (the "License") you may not use this file
* except in compliance with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the
* License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND,
* either express or implied. See the License for the specific language governing permissions
* and limitations under the License.
* =========================================================================================
*/

package kamon.instrumentation.armeria.client.timing;

import com.linecorp.armeria.common.logging.ClientConnectionTimings;
import com.linecorp.armeria.common.logging.RequestLog;
import kamon.Kamon;
import kamon.trace.Span;

import java.util.concurrent.TimeUnit;

/**
* Based on Armeria
* <a href="https://github.com/line/armeria/blob/master/brave/src/main/java/com/linecorp/armeria/client/brave/BraveClient.java">BraveClient</a>
* implementation.
*/
public final class Timing {

public static void takeTimings(RequestLog log, Span span) {
ClientConnectionTimings timings = log.connectionTimings();
if (timings != null) {

logTiming(span, "connection-acquire.start", "connection-acquire.end",
timings.connectionAcquisitionStartTimeMicros(),
timings.connectionAcquisitionDurationNanos());

if (timings.dnsResolutionDurationNanos() != -1) {
logTiming(span, "dns-resolve.start", "dns-resolve.end",
timings.dnsResolutionStartTimeMicros(),
timings.dnsResolutionDurationNanos());
}

if (timings.socketConnectDurationNanos() != -1) {
logTiming(span, "socket-connect.start", "socket-connect.end",
timings.socketConnectStartTimeMicros(),
timings.socketConnectDurationNanos());
}

if (timings.pendingAcquisitionDurationNanos() != -1) {
logTiming(span, "connection-reuse.start", "connection-reuse.end",
timings.pendingAcquisitionStartTimeMicros(),
timings.pendingAcquisitionDurationNanos());
}
}
}

private static void logTiming(Span span,
String startName,
String endName,
long startTimeMicros,
long durationNanos) {

final long startTimeNanos = TimeUnit.NANOSECONDS.convert(startTimeMicros, TimeUnit.MICROSECONDS);

span.mark(startName, Kamon.clock().toInstant(startTimeNanos));
span.mark(endName, Kamon.clock().toInstant(startTimeNanos + durationNanos));
}


}
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/* =========================================================================================
* Copyright © 2013-2020 the kamon project <http://kamon.io/>
*
* Licensed under the Apache License, Version 2.0 (the "License") you may not use this file
* except in compliance with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the
* License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND,
* either express or implied. See the License for the specific language governing permissions
* and limitations under the License.
* =========================================================================================
*/

package kamon.instrumentation.armeria.server;

import com.linecorp.armeria.common.HttpRequest;
import com.linecorp.armeria.common.HttpResponse;
import com.linecorp.armeria.server.HttpService;
import com.linecorp.armeria.server.ServiceRequestContext;
import com.linecorp.armeria.server.SimpleDecoratingHttpService;
import io.netty.util.AttributeKey;
import kamon.Kamon;
import kamon.context.Context;
import kamon.context.Storage;
import kamon.instrumentation.armeria.converters.KamonArmeriaMessageConverter;
import kamon.instrumentation.http.HttpServerInstrumentation;

import java.util.Map;

public class ArmeriaHttpServerDecorator extends SimpleDecoratingHttpService {
public static final AttributeKey<HttpServerInstrumentation.RequestHandler> REQUEST_HANDLER_TRACE_KEY =
AttributeKey.valueOf(HttpServerInstrumentation.RequestHandler.class, "REQUEST_HANDLER_TRACE");

private final Map<Integer, HttpServerInstrumentation> serverInstrumentationMap;

public ArmeriaHttpServerDecorator(HttpService delegate, Map<Integer, HttpServerInstrumentation> serverInstrumentations) {
super(delegate);
this.serverInstrumentationMap = serverInstrumentations;
}

@Override
public HttpResponse serve(ServiceRequestContext ctx, HttpRequest req) throws Exception {
final HttpServerInstrumentation httpServerInstrumentation = serverInstrumentationMap.get(req.uri().getPort());

if (httpServerInstrumentation != null) {

final HttpServerInstrumentation.RequestHandler requestHandler =
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use ctx.config().route().patternString() to set the operation name on the Spans and completely eliminate the name generator. Using the pattern string ensures we don't get cardinality issues and users won't need to add any operation mappings on the configuration file.

Plus, it works with all the possible service types as far as I could see!

Copy link
Author

Choose a reason for hiding this comment

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

I was trying to implement this suggested change like this

      ctx.log()
          .whenComplete()
          .thenAccept(log -> {
              final Context context = ctx.attr(REQUEST_HANDLER_TRACE_KEY).context();
              requestHandler.span().name(ctx.config().route().patternString());
              requestHandler.buildResponse(KamonArmeriaMessageConverter.toResponse(log),context);
              requestHandler.responseSent();
          });

And I've some concerns about this:

  • If a route does not exist in the server we will potentially generate N operation names (1 per route that does not exist in the server), because ctx.config().route().patternString() will return the request path even if the route does not exist. The current implementation ensures that the operation name will be set to unhandled(because of this) in this case. Probably we can check if we already set this operation name to the configured value for unhandled requests. I was wondering which will be a better approach, what do you think?
  • The client context, ClientRequestContext, does not have a method like ctx.config().route().patternString() so we'll need the operation name generator, right? I understand that users could have cardinality issues with the current implementation if they don't set operation mappings but I think there is nothing we can do here

Copy link
Contributor

Choose a reason for hiding this comment

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

Where we at here?

Copy link
Author

Choose a reason for hiding this comment

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

We are using the patternString() method provided by Armeria to generate operation names only in the server side, and we set the operation name as unhandled by default (it's configurable) if the route wasn't found.

In the client side we still use the custom operation name generator because we don't have an equivalent patternString() method. at least that is true until the version of Armeria that we are currently using which is not the latest.

httpServerInstrumentation.createHandler(KamonArmeriaMessageConverter.toRequest(req));

ctx.log()
.whenComplete()
.thenAccept(log -> {
final Context context = ctx.attr(REQUEST_HANDLER_TRACE_KEY).context();
/**
* This is true only when no configured Route was matched.
* Cases where the route is fallback should be managed here {@link kamon.instrumentation.armeria.server.HandleNotFoundMethodAdvisor#around}
*/
if (!ctx.config().route().isFallback()) {
requestHandler.span().name(ctx.config().route().patternString());
}

requestHandler.buildResponse(KamonArmeriaMessageConverter.toResponse(log), context);
requestHandler.responseSent();
});

try (Storage.Scope ignored = Kamon.storeContext(requestHandler.context())) {
ctx.setAttr(REQUEST_HANDLER_TRACE_KEY, requestHandler);
return unwrap().serve(ctx, req);
}
} else {
return unwrap().serve(ctx, req);
}

}

}


Loading