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

[NO-TICKET] Fix crashtracking sending parent pid/runtime-id when child crashes #4031

Merged
merged 4 commits into from
Oct 25, 2024

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Oct 25, 2024

What does this PR do?

This PR fixes a bug in the crashtracking component: because it cached all of the tags from creation, if a process forked, it would use the tags from the parent, which included a (now-incorrect) pid and runtime-id.

Motivation:

This makes sure we have accurate metadata in crashtracking reports.

Change log entry

(Internal change, not relevant for changelog)

Additional Notes:

N/A

How to test the change?

I've added test coverage for this change. It can also be observed by using

$ DD_CRASHTRACKING_ENABLED=true bundle exec ruby -e 'require "datadog/auto_instrument"; puts "Parent: #{Process.pid} #{Datadog::Core::Environment::Identity.id}"; fork { puts "Child: #{Process.pid} #{Datadog::Core::Environment::Identity.id}"; Process.kill("SEGV", Process.pid) }'

and validating that the correct pid/runtime-id from the child is sent in the metadata.

…d crashes

**What does this PR do?**

This PR fixes a bug in the crashtracking component: because it cached
all of the tags from creation, if a process forked, it would use the
tags from the parent, which included a (now-incorrect) pid and
runtime-id.

**Motivation:**

This makes sure we have accurate metadata in crashtracking reports.

**Additional Notes:**

N/A

**How to test the change?**

I've added test coverage for this change. It can also be observed by
using

```
$ DD_CRASHTRACKING_ENABLED=true bundle exec ruby -e 'require "datadog/auto_instrument"; puts "Parent: #{Process.pid} #{Datadog::Core::Environment::Identity.id}"; fork { puts "Child: #{Process.pid} #{Datadog::Core::Environment::Identity.id}"; Process.kill("SEGV", Process.pid) }'
```

and validating that the correct pid/runtime-id from the child
is sent in the metadata.
@ivoanjo ivoanjo requested a review from a team as a code owner October 25, 2024 12:45

it 'refreshes the latest settings' do
expect(Datadog).to receive(:configuration).and_return(:latest_settings)
expect(Datadog::Core::Crashtracking::TagBuilder).to receive(:call).with(:latest_settings).and_return([:latest_tags])
Copy link
Contributor

Choose a reason for hiding this comment

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

Code Quality Violation

Suggested change
expect(Datadog::Core::Crashtracking::TagBuilder).to receive(:call).with(:latest_settings).and_return([:latest_tags])
expect(Datadog::Core::Crashtracking::TagBuilder).to receive(:call).with(:latest_settings).and_return(%i[latest_tags])
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.

View in Datadog  Leave us feedback  Documentation

expect(Datadog).to receive(:configuration).and_return(:latest_settings)
expect(Datadog::Core::Crashtracking::TagBuilder).to receive(:call).with(:latest_settings).and_return([:latest_tags])
expect(described_class).to receive(:_native_start_or_update_on_fork).with(
hash_including(tags_as_array: [:latest_tags])
Copy link
Contributor

Choose a reason for hiding this comment

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

Code Quality Violation

Suggested change
hash_including(tags_as_array: [:latest_tags])
hash_including(tags_as_array: %i[latest_tags])
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.

View in Datadog  Leave us feedback  Documentation

Copy link
Contributor

@Strech Strech left a comment

Choose a reason for hiding this comment

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

LGTM 💯

spec/datadog/core/crashtracking/component_spec.rb Outdated Show resolved Hide resolved
Use `allow` instead of `expect` for test setup

Co-authored-by: Sergey Fedorov <oni.strech@gmail.com>
@pr-commenter
Copy link

pr-commenter bot commented Oct 25, 2024

Benchmarks

Benchmark execution time: 2024-10-25 13:35:08

Comparing candidate commit 06dfc27 in PR branch ivoanjo/fix-crashtracking-pid-runtimeid-on-fork with baseline commit 91d883f in branch master.

Found 2 performance improvements and 1 performance regressions! Performance is the same for 21 metrics, 2 unstable metrics.

scenario:profiler - sample timeline=false

  • 🟩 throughput [+0.471op/s; +0.524op/s] or [+7.401%; +8.236%]

scenario:tracing - Propagation - Datadog

  • 🟥 throughput [-3440.700op/s; -3340.000op/s] or [-10.231%; -9.932%]

scenario:tracing - Propagation - Trace Context

  • 🟩 throughput [+3555.631op/s; +3676.609op/s] or [+10.294%; +10.644%]

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.86%. Comparing base (91d883f) to head (06dfc27).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4031      +/-   ##
==========================================
- Coverage   97.86%   97.86%   -0.01%     
==========================================
  Files        1321     1321              
  Lines       79326    79329       +3     
  Branches     3934     3934              
==========================================
+ Hits        77631    77632       +1     
- Misses       1695     1697       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ivoanjo ivoanjo merged commit 321f513 into master Oct 25, 2024
269 of 270 checks passed
@ivoanjo ivoanjo deleted the ivoanjo/fix-crashtracking-pid-runtimeid-on-fork branch October 25, 2024 14:19
@github-actions github-actions bot added this to the 2.5.0 milestone Oct 25, 2024
@ivoanjo ivoanjo added core Involves Datadog core libraries dev/internal Other internal work that does not need to be included in the changelog labels Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries dev/internal Other internal work that does not need to be included in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants