-
Notifications
You must be signed in to change notification settings - Fork 1k
test otel span is passed from pekko routes to actors #12396
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
Closed
Closed
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
f53f4a9
test otel span is passed from pekko routes to actors
pjfanning e380241
stray imports
pjfanning ef58870
Update PekkoHttpTestWebServerWithActor.scala
pjfanning 1addd43
refactor test
pjfanning dd5ea77
Update PekkoHttpTestWebServerWithActor.scala
pjfanning File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
43 changes: 43 additions & 0 deletions
43
...st/java/io/opentelemetry/javaagent/instrumentation/pekkohttp/v1_0/PekkoHttpTestSetup.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| /* | ||
| * Copyright The OpenTelemetry Authors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package io.opentelemetry.javaagent.instrumentation.pekkohttp.v1_0; | ||
|
|
||
| import static io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpServerUsingTest.TEST_CLIENT_IP; | ||
| import static io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpServerUsingTest.TEST_USER_AGENT; | ||
|
|
||
| import io.opentelemetry.instrumentation.test.utils.PortUtils; | ||
| import io.opentelemetry.testing.internal.armeria.client.ClientFactory; | ||
| import io.opentelemetry.testing.internal.armeria.client.WebClient; | ||
| import io.opentelemetry.testing.internal.armeria.client.logging.LoggingClient; | ||
| import io.opentelemetry.testing.internal.armeria.common.HttpHeaderNames; | ||
| import java.time.Duration; | ||
|
|
||
| public final class PekkoHttpTestSetup { | ||
|
|
||
| private final int port; | ||
| private final WebClient client; | ||
|
|
||
| public PekkoHttpTestSetup() { | ||
| port = PortUtils.findOpenPort(); | ||
| client = | ||
| WebClient.builder() | ||
| .responseTimeout(Duration.ofMinutes(1)) | ||
| .writeTimeout(Duration.ofMinutes(1)) | ||
| .factory(ClientFactory.builder().connectTimeout(Duration.ofMinutes(1)).build()) | ||
| .setHeader(HttpHeaderNames.USER_AGENT, TEST_USER_AGENT) | ||
| .setHeader(HttpHeaderNames.X_FORWARDED_FOR, TEST_CLIENT_IP) | ||
| .decorator(LoggingClient.newDecorator()) | ||
| .build(); | ||
| } | ||
|
|
||
| public int getPort() { | ||
| return port; | ||
| } | ||
|
|
||
| public WebClient getClient() { | ||
| return client; | ||
| } | ||
| } | ||
45 changes: 45 additions & 0 deletions
45
...opentelemetry/javaagent/instrumentation/pekkohttp/v1_0/PekkoHttpServerWithActorTest.scala
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| /* | ||
| * Copyright The OpenTelemetry Authors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package io.opentelemetry.javaagent.instrumentation.pekkohttp.v1_0 | ||
|
|
||
| import java.net.URI | ||
|
|
||
| import io.opentelemetry.testing.internal.armeria.common.{ | ||
| AggregatedHttpRequest, | ||
| HttpMethod | ||
| } | ||
| import org.assertj.core.api.Assertions.assertThat | ||
| import org.junit.jupiter.api.{AfterAll, BeforeAll, Test, TestInstance} | ||
|
|
||
| @TestInstance(TestInstance.Lifecycle.PER_CLASS) | ||
| class PekkoHttpServerWithActorTest { | ||
|
|
||
| val setup = new PekkoHttpTestSetup() | ||
|
|
||
| @BeforeAll def setupOptions(): Unit = { | ||
| PekkoHttpTestWebServerWithActor.start(setup.getPort()) | ||
| } | ||
|
|
||
| @AfterAll def cleanup(): Unit = { | ||
| PekkoHttpTestWebServerWithActor.stop() | ||
| } | ||
|
|
||
| @Test def testSpan(): Unit = { | ||
| val address = new URI(s"http://localhost:${setup.getPort()}/") | ||
| val request = AggregatedHttpRequest.of( | ||
| HttpMethod.GET, | ||
| address.resolve("/test").toString | ||
| ) | ||
| val response = setup.getClient().execute(request).aggregate.join | ||
| assertThat(response.status.code).isEqualTo(200) | ||
| val responseText = response.contentUtf8 | ||
| val splits = responseText.split("\n") | ||
| assertThat(splits.length).isEqualTo(2) | ||
| val routeSpan = splits(0).substring(6, splits(0).length) | ||
| val actorSpan = splits(1).substring(6, splits(1).length) | ||
| assertThat(routeSpan).isEqualTo(actorSpan) | ||
| } | ||
| } |
67 changes: 67 additions & 0 deletions
67
...ntelemetry/javaagent/instrumentation/pekkohttp/v1_0/PekkoHttpTestWebServerWithActor.scala
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| /* | ||
| * Copyright The OpenTelemetry Authors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package io.opentelemetry.javaagent.instrumentation.pekkohttp.v1_0 | ||
|
|
||
| import io.opentelemetry.api.trace.Span | ||
| import org.apache.pekko.actor.{Actor, ActorSystem, Props} | ||
| import org.apache.pekko.http.scaladsl.Http | ||
| import org.apache.pekko.http.scaladsl.Http.ServerBinding | ||
| import org.apache.pekko.http.scaladsl.server.Directives._ | ||
| import org.apache.pekko.pattern.ask | ||
| import org.apache.pekko.util.Timeout | ||
|
|
||
| import scala.concurrent.{Await, ExecutionContext} | ||
| import scala.concurrent.duration.DurationInt | ||
|
|
||
| object PekkoHttpTestWebServerWithActor { | ||
| implicit val system: ActorSystem = ActorSystem("http-server-with-actor") | ||
| // needed for the future flatMap/onComplete in the end | ||
| implicit val executionContext: ExecutionContext = system.dispatcher | ||
|
|
||
| private case object TestMessage | ||
| private class SpanTestActor extends Actor { | ||
| def receive = { case TestMessage => | ||
| sender() ! spanSummary(Span.current()) | ||
| } | ||
| } | ||
|
|
||
| val spanTestActor = system.actorOf(Props[SpanTestActor]()) | ||
|
|
||
| var route = get { | ||
| path("test") { | ||
| complete { | ||
| val otelSummary = spanSummary(Span.current()) | ||
| spanTestActor.ask(TestMessage)(Timeout(5.seconds)).mapTo[String].map { | ||
| actorSummary => | ||
| s"Route=$otelSummary\nActor=$actorSummary" | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private var binding: ServerBinding = null | ||
|
|
||
| def start(port: Int): Unit = synchronized { | ||
| if (null == binding) { | ||
| binding = | ||
| Await.result(Http().bindAndHandle(route, "localhost", port), 10.seconds) | ||
| } | ||
| } | ||
|
|
||
| def stop(): Unit = synchronized { | ||
| if (null != binding) { | ||
| binding.unbind() | ||
| system.terminate() | ||
| binding = null | ||
| } | ||
| } | ||
|
|
||
| def spanSummary(span: Span): String = { | ||
| val spanId = span.getSpanContext().getSpanId() | ||
| val traceId = span.getSpanContext().getTraceId() | ||
| s"Span(traceId=$traceId, spanId=$spanId)" | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you consider extending
AbstractHttpServerUsingTestinstead of introducing this class?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AbstractHttpServerUsingTest carries a load of existing tests with it. I just want to add one new test that is independent of AbstractHttpServerUsingTest's existing tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WebClient setup in this class is copied from private code in this lib. If I am allowed to modify that code so that I can access then I can refactor or remove this new class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are mixing
AbstractHttpServerUsingTestwithAbstractHttpServerTest.AbstractHttpServerUsingTestcontains zero tests.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I need this new class is because I need a WebClient instance. AbstractHttpServerUsingTest does not create a WebClient.
HttpServerInstrumentationExtension creates a WebClient but it needs an InstrumentationTestRunner.
These are all major rabbit holes.
I can get rid of this class and just create my own WebClient in my test class. I just didn't want to write this in Scala but I will if this Java class is a problem for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically you'd extend the
AbstractHttpServerUsingTestand in the test register the extension withThen you can use the
WebClientin your test. You can also usetesting.runWithSpan(...)to create spans andtesting.waitAndAssertTraces(...)to verify that the spans have the correct structure instead of trying to encode trace and span info in the http response. I'd suggest you give it a try, it will be much nicer than what you currently have.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok - I'll abandon this
I don't think it is healthy to have such a high bar for tests.