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 #10233

Merged
merged 4 commits into from
Aug 26, 2024

Conversation

erikayasuda
Copy link
Contributor

@erikayasuda erikayasuda commented Aug 14, 2024

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

@erikayasuda erikayasuda added changelog/no-changelog A changelog entry is not required for this PR. backport 2.11 labels Aug 14, 2024
@erikayasuda erikayasuda changed the title chore(crashtracking): Enable wait_for_receiver field to be configured via env var chore(crashtracking): enable wait_for_receiver field to be configured via env var Aug 14, 2024
Copy link
Contributor

github-actions bot commented Aug 14, 2024

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

Copy link
Contributor

@sanchda sanchda left a comment

Choose a reason for hiding this comment

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

From my side, looks good. Once the style nags pass this should be good to go 👍

@erikayasuda erikayasuda marked this pull request as ready for review August 14, 2024 20:39
@erikayasuda erikayasuda requested review from a team as code owners August 14, 2024 20:39
@sanchda sanchda self-requested a review August 14, 2024 20:40
@erikayasuda erikayasuda enabled auto-merge (squash) August 14, 2024 20:41
@datadog-dd-trace-py-rkomorn
Copy link

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

Datadog Report

Branch report: erikayasuda/crashtracker-wait-for-receiver-fix
Commit report: 70a4867
Test service: dd-trace-py

✅ 0 Failed, 122673 Passed, 19749 Skipped, 4h 1m 55.42s Total duration (2m 19s time saved)

@erikayasuda
Copy link
Contributor Author

Just tried it out with the updated system tests, and it's not passing 😞 Going to investigate

@pr-commenter
Copy link

pr-commenter bot commented Aug 14, 2024

Benchmarks

Benchmark execution time: 2024-08-26 14:19:58

Comparing candidate commit 70a4867 in PR branch erikayasuda/crashtracker-wait-for-receiver-fix with baseline commit ee75b12 in branch main.

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

@taegyunkim taegyunkim force-pushed the erikayasuda/crashtracker-wait-for-receiver-fix branch from 7a75d52 to fcd8b88 Compare August 15, 2024 19:54
@erikayasuda erikayasuda enabled auto-merge (squash) August 26, 2024 13:41
auto-merge was automatically disabled August 26, 2024 14:37

Pull request was closed

@erikayasuda erikayasuda reopened this Aug 26, 2024
@erikayasuda erikayasuda enabled auto-merge (squash) August 26, 2024 14:37
@erikayasuda erikayasuda merged commit bb82a8e into main Aug 26, 2024
339 of 341 checks passed
@erikayasuda erikayasuda deleted the erikayasuda/crashtracker-wait-for-receiver-fix branch August 26, 2024 15:04
github-actions bot pushed a commit that referenced this pull request Aug 26, 2024
…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 bot pushed a commit that referenced this pull request Aug 26, 2024
…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)
Copy link
Contributor

The backport to 2.12 failed:

Validation Failed: {"resource":"Label","code":"unprocessable","field":"data","message":"Could not resolve to a node with the global id of 'PR_kwDOA6uE5s55c9Rw'."}

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.12 2.12
# Navigate to the new working tree
cd .worktrees/backport-2.12
# Create a new branch
git switch --create backport-10233-to-2.12
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 bb82a8e177cfb89a0b8e437e724fa0c06df59a61
# Push it to GitHub
git push --set-upstream origin backport-10233-to-2.12
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.12

Then, create a pull request where the base branch is 2.12 and the compare/head branch is backport-10233-to-2.12.

erikayasuda added a commit that referenced this pull request Aug 26, 2024
…ed via env var [backport 2.11] (#10380)

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](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)

Co-authored-by: erikayasuda <153395705+erikayasuda@users.noreply.github.com>
taegyunkim pushed a commit that referenced this pull request Aug 28, 2024
…ed via env var [backport 2.12] (#10381)

Backport bb82a8e from #10233 to 2.12.

### 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)

Co-authored-by: erikayasuda <153395705+erikayasuda@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.11 backport 2.12 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