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

chore(crashtracking): enable wait_for_receiver field to be configured via env var [backport 2.11] #10380

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 26, 2024

Backport bb82a8e from #10233 to 2.11.

Change Overview

This PR enables the wait_for_receiver field to be set via DD_CRASHTRACKING_WAIT_FOR_RECEIVER environment variable. If unset, the default is true.

Motivation

System-tests for crashtracking exposed that the Python crashtracker needs the wait_for_receiver field to be set to true to capture crashes and reliably send telemetry events to the agent. While set to false, telemetry events for crashes would work very rarely. When set to true, this would become reliably consistent.

The value for wait_for_receiver is unset in libdatadog, which means it is set to false by default. This default needs to be updated on the libdatadog end. However, in order to unblock crashtracking functionality for dd-trace-py, we are also enabling this to be configured on the dd-trace-py side as well.

Checklist

  • PR author has checked that all the criteria below are met
  • The PR description includes an overview of the change
  • The PR description articulates the motivation for the change
  • The change includes tests OR the PR description describes a testing strategy
  • The PR description notes risks associated with the change, if any
  • Newly-added code is easy to change
  • The change follows the library release note guidelines
  • The change includes or references documentation updates if necessary
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Reviewer has checked that all the criteria below are met
  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Newly-added code is easy to change
  • Release note makes sense to a user of the library
  • If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy

…ed via env var (#10233)

### Change Overview
This PR enables the `wait_for_receiver` field to be set via
`DD_CRASHTRACKING_WAIT_FOR_RECEIVER` environment variable. If unset, the
default is `true`.

### Motivation
[System-tests for
crashtracking](DataDog/system-tests#2884)
exposed that the Python crashtracker needs the `wait_for_receiver` field
to be set to `true` to capture crashes and reliably send telemetry
events to the agent. While set to `false`, telemetry events for crashes
would work very rarely. When set to `true`, this would become reliably
consistent.

The value for `wait_for_receiver` is [unset in
`libdatadog`](https://github.com/DataDog/libdatadog/blob/16528ffee456f7af5fe9ad80a6294fb5dcd38918/crashtracker/src/shared/configuration.rs#L28),
which means it is set to `false` by default. This default needs to be
updated on the `libdatadog` end. However, in order to unblock
crashtracking functionality for `dd-trace-py`, we are also enabling this
to be configured on the `dd-trace-py` side as well.

## Checklist
- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] Reviewer has checked that all the criteria below are met
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, author has acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment
- Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

(cherry picked from commit bb82a8e)
@github-actions github-actions bot requested review from a team as code owners August 26, 2024 15:05
@github-actions github-actions bot added the changelog/no-changelog A changelog entry is not required for this PR. label Aug 26, 2024
@datadog-dd-trace-py-rkomorn
Copy link

datadog-dd-trace-py-rkomorn bot commented Aug 26, 2024

Datadog Report

Branch report: backport-10233-to-2.11
Commit report: 9d59731
Test service: dd-trace-py

✅ 0 Failed, 146102 Passed, 32044 Skipped, 8h 9m 56.11s Total duration (1m 22.51s time saved)
⌛ 1 Performance Regression

⌛ Performance Regressions vs Default Branch (1)

  • test_data_streams_payload_size[key_and_length2-payload_and_length2] - test_kafka.py 1m 30.01s (+1m 26.66s, +2593%) - Details

@erikayasuda erikayasuda enabled auto-merge (squash) August 26, 2024 15:32
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 10.57%. Comparing base (9c353ce) to head (9d59731).
Report is 21 commits behind head on 2.11.

Files Patch % Lines
ddtrace/settings/crashtracker.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             2.11   #10380       +/-   ##
===========================================
- Coverage   34.18%   10.57%   -23.62%     
===========================================
  Files        1385     1385               
  Lines      129280   129474      +194     
===========================================
- Hits        44191    13687    -30504     
- Misses      85089   115787    +30698     

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

@pr-commenter
Copy link

pr-commenter bot commented Aug 26, 2024

Benchmarks

Benchmark execution time: 2024-08-26 15:41:40

Comparing candidate commit 9d59731 in PR branch backport-10233-to-2.11 with baseline commit 03cfc4d in branch 2.11.

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

auto-merge was automatically disabled August 26, 2024 16:51

Pull request was closed

@erikayasuda erikayasuda reopened this Aug 26, 2024
Copy link
Contributor Author

CODEOWNERS have been resolved as:

ddtrace/internal/core/crashtracking.py                                  @DataDog/apm-core-python
ddtrace/internal/datadog/profiling/crashtracker/_crashtracker.pyi       @DataDog/profiling-python
ddtrace/internal/datadog/profiling/crashtracker/_crashtracker.pyx       @DataDog/profiling-python
ddtrace/internal/datadog/profiling/dd_wrapper/include/crashtracker.hpp  @DataDog/profiling-python
ddtrace/internal/datadog/profiling/dd_wrapper/include/crashtracker_interface.hpp  @DataDog/profiling-python
ddtrace/internal/datadog/profiling/dd_wrapper/src/crashtracker.cpp      @DataDog/profiling-python
ddtrace/internal/datadog/profiling/dd_wrapper/src/crashtracker_interface.cpp  @DataDog/profiling-python
ddtrace/settings/crashtracker.py                                        @DataDog/apm-core-python

@erikayasuda erikayasuda enabled auto-merge (squash) August 26, 2024 17:16
auto-merge was automatically disabled August 26, 2024 17:53

Pull request was closed

@erikayasuda erikayasuda reopened this Aug 26, 2024
@erikayasuda erikayasuda enabled auto-merge (squash) August 26, 2024 19:06
@erikayasuda erikayasuda merged commit b95a737 into 2.11 Aug 26, 2024
287 of 292 checks passed
@erikayasuda erikayasuda deleted the backport-10233-to-2.11 branch August 26, 2024 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants