-
Notifications
You must be signed in to change notification settings - Fork 873
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
Ratpack services OpenTelemetry #7477
Conversation
|
||
app.address | ||
latch.await() | ||
def spanData = spanExporter.finishedSpanItems.find { it.name == "a-span" } |
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 suspect that doing it this way will give you a flaky test that occasionally fails because spans haven't been received yet. Any reason why you are not using assertTraces
as all other tests are?
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 am not familiar with the custom DSL and does not looks to be used in groovy
tests
AFAIK, it have not been flaky in these 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.
This seems to be a weird test that doesn't extend InstrumentationSpecification
so assertTraces
isn't available. I guess you could add new PollingConditions().eventually {}
around asserts as other tests in this file do.
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've added the eventually
as I am not familiar with the InMemorySpanExporter
but I think that those tests are deterministic with the CountDownLatch
Other tests in the file are using eventually because the HttpClient is async and requires eventually
, but in this case, the CountDownLatch
does the countDown
in the Promise subscription which makes it deterministic.
The only scenario that I can think of is if publishing to InMemorySpanExporter
is async. As I am not too familiar with the InMemory, let's leave the eventually
?
@mateuszrzeszutek @anuraaga @trask do you have any feedback 🙏 ? I remember you reviewed some previous PRs about Ratpack |
@@ -28,7 +28,8 @@ public HttpClient instrument(HttpClient httpClient) throws Exception { | |||
httpClientSpec -> { | |||
httpClientSpec.requestIntercept( | |||
requestSpec -> { | |||
Context parentOtelCtx = Context.current(); | |||
Context parentOtelCtx = | |||
Execution.current().maybeGet(Context.class).orElse(Context.current()); |
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.
hi @jsalinaspolo! is this still needed if you are using RatpackTelemetry.getOpenTelemetryExecInterceptor()
?
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.
is this still needed if you are using
RatpackTelemetry.getOpenTelemetryExecInterceptor()
?
Unforunelty, yes, because the OpenTelemetryExecInterceptor
only works within Ratpack handlers
The PR helps when we use the Ratpack HttpClient in Ratpack Services, which runs in a different Execution, so the compute
thread is not blocked.
The Ratpack service will add the OpenTelemetry Context to the Execution
when Instrumentation is required.
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.
is it possible to configure Ratpack Services to use the OpenTelemetryExecInterceptor?
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.
Unforunelty, yes, because the OpenTelemetryExecInterceptor only works within Ratpack handlers
What I meant is that works because the OpenTelemetryServerHandler
adds the OpenTelemetry Context into the Ratpack Context
is it possible to configure Ratpack Services to use the OpenTelemetryExecInterceptor?
We could use something similar to OpenTelemetryServerHandler
but I do not think that it is what we would want 🤔
The idea of the OpenTelemetryServerHandler
is to add instrumentation into the Handlers
(the entry point is an HTTP request) and add the OpenTelemetry Context to the Ratpack Execution, so other instrumentation is propagated within the Handler
Ratpack Services, most of the time, you want to run things in the background using something different than the compute
threads (so the compute threads are not blocked and can receive requests). Hence, many times the Service will be running things in backgrounds like Kafka Consumers, Kafka Streams, Rabbit Consumers, or just background processes with loops.
Therefore, might not make sense to have a parent span within the Service (as it does with the Handler) but each of the iterations/events running in the service might want to create a Span
with or without a parent (if the context is propagated in the events, and so on)
I think that for Services, it makes sense to have the flexibility to decide when to create the Span that aggregates the "iteration"
thx @jsalinaspolo! |
While using Ratpack Handlers, the context is added by the
OpenTelemetryServerHandler
While using Ratpack Services, we might need to add manually the OT Context into the Ratpack Execution and try to get it from the Execution in the Ratpack
HttpClient