-
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
Conversation
| import io.opentelemetry.testing.internal.armeria.common.HttpHeaderNames; | ||
| import java.time.Duration; | ||
|
|
||
| public final class PekkoHttpTestSetup { |
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 AbstractHttpServerUsingTest instead 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 AbstractHttpServerUsingTest with AbstractHttpServerTest. AbstractHttpServerUsingTest contains 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 AbstractHttpServerUsingTest and in the test register the extension with
@RegisterExtension
static final InstrumentationExtension testing = HttpServerInstrumentationExtension.forAgent();Then you can use the WebClient in your test. You can also use testing.runWithSpan(...) to create spans and testing.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.
Based on POC in #5138
This test seems to indicate that the OTel Span is shared and that Pekko actor operates in same span as the Pekko HTTP route.
I may have made a mistake in the test. If this test looks ok, it might be useful to merge it as a regression test.