[SDTEST-409] Send telemetry events in background thread to remove blocking HTTP calls from critical path #3718
Conversation
anmarchenko
changed the title
anmarchenko
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3718 +/- ##
========================================
Coverage 98.10% 98.10%
========================================
Files 1225 1227 +2
Lines 72866 73014 +148
Branches 3493 3508 +15
========================================
+ Hits 71485 71632 +147
- Misses 1381 1382 +1 ☔ View full report in Codecov by Sentry. |
anmarchenko
changed the title
anmarchenko
changed the title
| end | ||
| ) | ||
| end | ||
|
|
||
| # Should only be called the first time components are built |
There was a problem hiding this comment.
We are able to remove this logic because Telemetry::Worker now handles that app-started is sent once. Note that before it was not guaranteed that app-started event was sent: if agent URL was not configured via environment variable, telemetry would silently fail to start. This is handled correctly now.
There was a problem hiding this comment.
Love removing "special behavior" like this!
| end | ||
| ) | ||
| end | ||
|
|
||
| # Should only be called the first time components are built |
There was a problem hiding this comment.
Love removing "special behavior" like this!
|
Nice! Here's my original reproducer for the blocking calls issue: require 'socket'
require 'datadog'
server = TCPServer.new(8126) # fake agent which never replies
puts Datadog::Tracing.enabled?Here's what I get in master: and here's what I get with this PR: The printing of |
|
@ivoanjo thanks for the reproducer! I have a suspicion that workers shutdown might not work properly if worker is blocked by IO. I will play around with shorter HTTP timeout values and reproducer you provided (I think 30 seconds timeout for telemetry is too long anyway). |
|
@ivoanjo this was a good catch, the timeout was indeed too long for telemetry use case, I lowered HTTP timeout to 2 seconds, the result: |
…re stopping worker
…pendency collection event logic, move it to the Worker
anmarchenko
force-pushed
the
anmarchenko/telemetry_send_events_in_background_thread
branch
from
24a0409
to
5729921
Compare
anmarchenko
deleted the
anmarchenko/telemetry_send_events_in_background_thread
branch
What does this PR do?
Moves all telemetry events sending to the background thread. Event "app-started" is now being sent when the
Telemetry::Worker(only one successful event dispatch per application run), no additional logic to determine when to calltelemetry.started!is required.Notable changes include:
Telemetry::Clientis renamed toTelemetry::Componentto better reflect its purpose: it is no longer dispatches HTTP requests, but provides a public API to enqueue them.Telemetry::Heartbeatis renamed toTelemetry::Worker. Now it includesCore::Workers::Queueas well as buffer with limited capacity.
OnlyOnceutil:OnlyOnceSuccessful. It shares most of its functionality withOnlyOncewith important difference: it resetsran_oncevar if the block returns falsey value such asnilorfalse. This ensures thatapp-startedevent is sent once, successfully (before that, if agent was misconfigured on application start, telemetry would fail to start correctly)Core::Configurationno longer includes custom logic to determine whether to calltelemetry.started!: now it is handled internally byTelemetry::Worker, there is no need for Core to handle specific component's lifecycle.Motivation:
Currently
Telemetry::Clientclass performs HTTP requests to the agent synchronously which can cause delays to the instrumented application.How to test the change?
Tested using empty rails app, here are the debug logs: