Add parametric tests for crashtracking #2884

kevingosse · 2024-08-14T17:20:53Z

Motivation

Validates the behavior of crashtracking across tracers.

Changes

Added a new test scenario (crashtracking)
Added a method on test_agent to trigger a crash. It does so by calling the /trace/crash HTTP endpoint (needs to be implemented for each language)
Used .NET to validate the test

Workflow

⚠️ Create your PR as draft ⚠️
Work on you PR until the CI passes (if something not related to your task is failing, you can ignore it)
Mark it as ready for review
- Test logic is modified? -> Get a review from RFC owner. We're working on refining the codeowners file quickly.
- Framework is modified, or non obvious usage of it -> get a review from R&P team

🚀 Once your PR is reviewed, you can merge it!

🛟 #apm-shared-testing 🛟

Reviewer checklist

If PR title starts with [<language>], double-check that only <language> is impacted by the change
No system-tests internal is modified. Otherwise, I have the approval from R&P team
CI is green, or failing jobs are not related to this change (and you are 100% sure about this statement)
A docker base image is modified?
- the relevant build-XXX-image label is present
A scenario is added (or removed)?
- Get a review from R&P team

Kyle-Verhoog

lgtm

lucaspimentel · 2024-08-16T15:59:42Z

utils/_context/_scenarios/parametric.py

@@ -469,6 +469,7 @@ def dotnet_library_factory():
 ENV CORECLR_ENABLE_PROFILING=0
 ENV CORECLR_PROFILER={{846F5F1C-F9AE-4B07-969E-05C26BC060D8}}
 ENV CORECLR_PROFILER_PATH=/opt/datadog/Datadog.Trace.ClrProfiler.Native.so
+ENV LD_PRELOAD=/opt/datadog/continuousprofiler/Datadog.Linux.ApiWrapper.x64.so


Is it ok to add this to all .NET parametric tests, or should we add it to the new tests only?

I think it's better to add it to all tests. My reasoning is that it shouldn't impact the other tests, so that's a good way to make sure it doesn't.

lucaspimentel · 2024-08-16T16:02:24Z

utils/build/docker/dotnet/parametric/Endpoints/ApmTestApi.cs

+    {
+        var thread = new Thread(() =>
+        {
+            throw new BadImageFormatException("Expected");


Just curious, why throw the exception from another thread?

If I throw it from the current thread, aspnetcore will catch it and it won't crash the app

colin-higgins · 2024-08-16T17:45:07Z

tests/parametric/test_crashtracking.py

+        event = test_agent.wait_for_telemetry_event("logs", wait_loops=400)
+        assert self.is_crash_report(event)
+
+    @missing_feature(context.library == "dotnet", reason="Not implemented")


Why not implemented?

Currently .NET uses the wrong environment variable, there is a pending PR to fix that

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

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

…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>

…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>

kevingosse added 5 commits August 13, 2024 18:39

WIP

7be0e5c

Make it work

51b0982

Check for the tag

7d8f217

disable tests

282cc8d

Revert changes for python

15bccf0

kevingosse requested review from Kyle-Verhoog and a team as code owners August 14, 2024 17:20

kevingosse requested a review from wconti27 August 14, 2024 17:20

kevingosse added the run-parametric-scenario label Aug 14, 2024

Kyle-Verhoog approved these changes Aug 14, 2024

View reviewed changes

erikayasuda mentioned this pull request Aug 14, 2024

chore(crashtracking): enable wait_for_receiver field to be configured via env var DataDog/dd-trace-py#10233

Merged

2 tasks

kevingosse added 4 commits August 16, 2024 11:50

Use the signum tag to identify the crash

926c9e6

Try making the linter happy

e4f6b5f

Getting there

f244119

Getting there

72c1d73

lucaspimentel reviewed Aug 16, 2024

View reviewed changes

lucaspimentel approved these changes Aug 16, 2024

View reviewed changes

colin-higgins reviewed Aug 16, 2024

View reviewed changes

colin-higgins approved these changes Aug 16, 2024

View reviewed changes

kevingosse merged commit 0dc100a into main Aug 19, 2024
322 of 328 checks passed

kevingosse deleted the kevin/crashtracking branch August 19, 2024 08:51

github-actions bot mentioned this pull request Aug 26, 2024

chore(crashtracking): enable wait_for_receiver field to be configured via env var [backport 2.11] DataDog/dd-trace-py#10380

Merged

2 tasks

github-actions bot mentioned this pull request Aug 26, 2024

chore(crashtracking): enable wait_for_receiver field to be configured via env var [backport 2.12] DataDog/dd-trace-py#10381

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parametric tests for crashtracking #2884

Add parametric tests for crashtracking #2884

kevingosse commented Aug 14, 2024 •

edited by colin-higgins

Loading

Kyle-Verhoog left a comment

lucaspimentel Aug 16, 2024 •

edited

Loading

kevingosse Aug 19, 2024

lucaspimentel Aug 16, 2024

kevingosse Aug 19, 2024

colin-higgins Aug 16, 2024

kevingosse Aug 19, 2024

Add parametric tests for crashtracking #2884

Add parametric tests for crashtracking #2884

Conversation

kevingosse commented Aug 14, 2024 • edited by colin-higgins Loading

Motivation

Changes

Workflow

Reviewer checklist

Kyle-Verhoog left a comment

Choose a reason for hiding this comment

lucaspimentel Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

kevingosse Aug 19, 2024

Choose a reason for hiding this comment

lucaspimentel Aug 16, 2024

Choose a reason for hiding this comment

kevingosse Aug 19, 2024

Choose a reason for hiding this comment

colin-higgins Aug 16, 2024

Choose a reason for hiding this comment

kevingosse Aug 19, 2024

Choose a reason for hiding this comment

kevingosse commented Aug 14, 2024 •

edited by colin-higgins

Loading

lucaspimentel Aug 16, 2024 •

edited

Loading