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.

Sign up for GitHub

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

Jump to bottom

Prevent telemetry deadlock with no-op #3910

Merged
merged 5 commits into from
Sep 13, 2024
Merged

Prevent telemetry deadlock with no-op #3910

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
503bb5a
Prevent deadlock with no-op for telemetry logger
TonyCTHsu Sep 13, 2024
cabb511
Change to warn
TonyCTHsu Sep 13, 2024
d5da8b0
Update comment
TonyCTHsu Sep 13, 2024
19c88eb
Fix typo
TonyCTHsu Sep 13, 2024
f64ae44
Update comment
TonyCTHsu Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions lib/datadog/core/telemetry/logger.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@
module Datadog
module Core
module Telemetry
# === INTRENAL USAGE ONLY ===
# === INTERNAL USAGE ONLY ===
#
# Report telemetry logs via delegating to the telemetry component instance via mutex.
ivoanjo marked this conversation as resolved.
Show resolved Hide resolved
#
# IMPORTANT: Invoking this method during the lifecycle of component initialization will
# cause a non-recoverable deadlock
# be no-op instead.
Comment on lines 10 to +11
Copy link
Member

Choose a reason for hiding this comment

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

While causing a deadlock is really bad, I think we should plan to fix the underlying problem. E.g. I think the core should provide to components the service of making telemetry available very early on; it seems reasonable for components to want to report data during initialization.

#
# For developer using this module:
# read: lib/datadog/core/telemetry/logging.rb
Expand All @@ -25,7 +25,24 @@ def error(description)
private

def instance
Datadog.send(:components).telemetry
# Component initialization uses a mutex to avoid having concurrent initialization.
# Trying to access the telemetry component during initialization (specifically:
# from the thread that's actually doing the initialization) would cause a deadlock,
# since accessing the components would try to recursively lock the mutex.
#
# To work around this, we use allow_initialization: false to avoid triggering this issue.
#
# The downside is: this leaves us unable to report telemetry during component initialization.
components = Datadog.send(:components, allow_initialization: false)

if components && components.telemetry
components.telemetry
else
Datadog.logger.warn(
'Fail to send telemetry log before components initialization or within components lifecycle'
)
nil
end
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/datadog/core/telemetry/logging.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
module Datadog
module Core
module Telemetry
# === INTRENAL USAGE ONLY ===
# === INTERNAL USAGE ONLY ===
#
# Logging interface for sending telemetry logs... so we can fix them.
#
Expand Down
5 changes: 5 additions & 0 deletions spec/datadog/core/telemetry/logger_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
telemetry = instance_double(Datadog::Core::Telemetry::Component)
allow(Datadog.send(:components)).to receive(:telemetry).and_return(telemetry)
expect(telemetry).to receive(:report).with(exception, level: :error, description: 'Oops...')
expect(Datadog.logger).not_to receive(:warn)

expect do
described_class.report(exception, level: :error, description: 'Oops...')
Expand All @@ -22,6 +23,7 @@
telemetry = instance_double(Datadog::Core::Telemetry::Component)
allow(Datadog.send(:components)).to receive(:telemetry).and_return(telemetry)
expect(telemetry).to receive(:report).with(exception, level: :error, description: nil)
expect(Datadog.logger).not_to receive(:warn)

expect do
described_class.report(exception)
Expand All @@ -34,6 +36,7 @@
it do
exception = StandardError.new
allow(Datadog.send(:components)).to receive(:telemetry).and_return(nil)
expect(Datadog.logger).to receive(:warn).with(/Fail to send telemetry log/)

expect do
described_class.report(exception, level: :error, description: 'Oops...')
Expand All @@ -48,6 +51,7 @@
telemetry = instance_double(Datadog::Core::Telemetry::Component)
allow(Datadog.send(:components)).to receive(:telemetry).and_return(telemetry)
expect(telemetry).to receive(:error).with('description')
expect(Datadog.logger).not_to receive(:warn)

expect { described_class.error('description') }.not_to raise_error
end
Expand All @@ -56,6 +60,7 @@
context 'when there is no telemetry component configured' do
it do
allow(Datadog.send(:components)).to receive(:telemetry).and_return(nil)
expect(Datadog.logger).to receive(:warn).with(/Fail to send telemetry log/)

expect { described_class.error('description') }.not_to raise_error
end
Expand Down
Loading