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

try to handle issues in Pekko tracing when the scheduler is used #12359

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

pjfanning
Copy link
Contributor

@pjfanning pjfanning commented Sep 29, 2024

Relates to #11755

The code is based on @wsargent 's workaround in https://github.com/wsargent/opentelemetry-with-scala-futures and includes the test submitted by @Dogacel

@pjfanning pjfanning requested a review from a team as a code owner September 29, 2024 23:10
@pjfanning pjfanning marked this pull request as draft September 29, 2024 23:10
@@ -32,4 +39,23 @@ class PekkoHttpServerInstrumentationTestAsync
super.configure(options)
options.setTestHttpPipelining(false)
}

@Test
def successfulPostRequestWithParent(): Unit = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Test name indicates that you are testing something completely different than what your true goal is. Test should be name appropriately. Parts that you don't need should be removed, do you really need the tracePARENT stuff? Why not send a simple get? I'd guess that for what you want to test you don't need to make a http request at all, perhaps you could skip that altogether?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look into rewriting this test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@laurit thanks for looking at the PR so quickly. I wrote a new test that does not use HTTP.

@pjfanning pjfanning marked this pull request as ready for review September 30, 2024 18:50
class names

add tests

Update PekkoScheduleInstrumentation.java

fix matcher

ExecutionContextWrapper

Update PekkoHttpTestAsyncWebServer.scala

extend test

remove ExecutionContextWrapper

remove PekkoScheduleOnceInstrumentation

Create PekkoSchedulerTest.scala

remove new http test

spotless

Update PekkoSchedulerTest.scala
@trask trask merged commit 2cb3961 into open-telemetry:main Oct 2, 2024
56 checks passed
@pjfanning pjfanning deleted the pekko-schedule branch October 2, 2024 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants