Fix deadlock when sending telemetry logs #3886
Conversation
github-actions
bot
added
appsec
TonyCTHsu
force-pushed
the
tonycthsu/avoid-telemetry-deadlock
branch
from
06f3c73
to
934d4c2
Compare
BenchmarksBenchmark execution time: 2024-09-06 08:52:28 Comparing candidate commit 9868ed2 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 23 metrics, 2 unstable metrics. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3886 +/- ##
=======================================
Coverage 97.85% 97.85%
=======================================
Files 1277 1279 +2
Lines 76392 76489 +97
Branches 3744 3745 +1
=======================================
+ Hits 74757 74852 +95
- Misses 1635 1637 +2 ☔ View full report in Codecov by Sentry. |
TonyCTHsu
force-pushed
the
tonycthsu/avoid-telemetry-deadlock
branch
from
934d4c2
to
a4d99a1
Compare
TonyCTHsu
force-pushed
the
tonycthsu/avoid-telemetry-deadlock
branch
from
7b61550
to
80356eb
Compare
TonyCTHsu
changed the title
TonyCTHsu
added
the
dev/internal
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
| end | ||
|
|
||
| def required_addresses | ||
| [:required_addresses] |
There was a problem hiding this comment.
⚪ Code Quality Violation
| [:required_addresses] | |
| %i[required_addresses] |
Consider using the %i syntax instead (...read more)
The rule "Prefer %i to the literal array syntax" is a guideline that encourages the use of the %i syntax for arrays of symbols. This is a part of the Ruby style guide that aims to promote conciseness and readability.
Symbols are immutable, reusable objects often used in Ruby instead of strings when the value does not need to be changed. When declaring an array of symbols, using the %i syntax can make your code cleaner and easier to read.
To adhere to this rule, instead of declaring an array of symbols using the literal array syntax like [:foo, :bar, :baz], use the %i syntax like %i[foo bar baz]. It's a good practice to consistently use %i for arrays of symbols as it enhances code readability and maintainability.
|
|
||
| expect(processor).to be_ready | ||
| expect(processor.diagnostics).to eq(:handle_diagnostics) | ||
| expect(processor.addresses).to eq([:required_addresses]) |
There was a problem hiding this comment.
⚪ Code Quality Violation
| expect(processor.addresses).to eq([:required_addresses]) | |
| expect(processor.addresses).to eq(%i[required_addresses]) |
Consider using the %i syntax instead (...read more)
The rule "Prefer %i to the literal array syntax" is a guideline that encourages the use of the %i syntax for arrays of symbols. This is a part of the Ruby style guide that aims to promote conciseness and readability.
Symbols are immutable, reusable objects often used in Ruby instead of strings when the value does not need to be changed. When declaring an array of symbols, using the %i syntax can make your code cleaner and easier to read.
To adhere to this rule, instead of declaring an array of symbols using the literal array syntax like [:foo, :bar, :baz], use the %i syntax like %i[foo bar baz]. It's a good practice to consistently use %i for arrays of symbols as it enhances code readability and maintainability.
Motivation:
This PR fix a bug introduced in: #3873
To reproduce:
DD_APPSEC_RULES='/weblog/wtf.json' bundle exec rails sThis runs a rails server with an invalid appsec rules and deadlock occurs when trying to report telemetry.
Although we rescue the deadlock, it is still a non-recoverable error as it left the application in a broken state which leads to unexpected error further downstream.
What does this PR do?
The error is mitigated by implementing dependency injection of the telemetry component instance instead of referencing the global telemetry component instance via mutex(from
safely_synchronize)The not so good part of it: Lots of code during components lifecycle are pure function/static, instead of object oriented. This means the telemetry component instance must be injected (because the function depends on it) to all the downstream functions. It leads to increased verbosity and shotgun surgery.
In order to make dependency injection work, I also changed to following:
:errorforlevelonreportmethod to decrease verbosity for callers.Telemetry::LoggingforTelemetry::Component, so that the component instance is able to report logs directly. By injecting the component instance, it could be used to repot other kinds of telemetry data when needed.Telemetry::Loggingdirectly, replace those byTelemetry::Loggerwhich delegates to the global telemetry component. There is still risk causing deadlock with this module.AppSec::Processorclass methods to private instance methods in order to shared the telemetry component instance.