Normalize the environment variable names used by crashtracking #5898
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewlock
approved these changes
Aug 14, 2024
TonyCTHsu
approved these changes
Aug 14, 2024
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing the following branches/commits: Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). |
Datadog ReportBranch report: ✅ 0 Failed, 348156 Passed, 2084 Skipped, 16h 12m 58.65s Total Time |
Benchmarks Report for tracer 🐌Benchmarks for #5898 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SpanBenchmark - Faster 🎉 Same allocations ✔️
|
| Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net472 | 1.133 | 634.79 | 560.43 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | StartFinishSpan |
net6.0 | 399ns | 0.0943ns | 0.34ns | 0.00803 | 0 | 0 | 576 B |
| master | StartFinishSpan |
netcoreapp3.1 | 557ns | 1.16ns | 4.49ns | 0.00796 | 0 | 0 | 576 B |
| master | StartFinishSpan |
net472 | 635ns | 0.427ns | 1.65ns | 0.0915 | 0 | 0 | 578 B |
| master | StartFinishScope |
net6.0 | 493ns | 1.19ns | 4.62ns | 0.00964 | 0 | 0 | 696 B |
| master | StartFinishScope |
netcoreapp3.1 | 748ns | 0.514ns | 1.99ns | 0.0094 | 0 | 0 | 696 B |
| master | StartFinishScope |
net472 | 888ns | 2.64ns | 10.2ns | 0.104 | 0 | 0 | 658 B |
| #5898 | StartFinishSpan |
net6.0 | 399ns | 0.372ns | 1.44ns | 0.00819 | 0 | 0 | 576 B |
| #5898 | StartFinishSpan |
netcoreapp3.1 | 622ns | 0.485ns | 1.82ns | 0.00777 | 0 | 0 | 576 B |
| #5898 | StartFinishSpan |
net472 | 560ns | 0.362ns | 1.25ns | 0.0918 | 0 | 0 | 578 B |
| #5898 | StartFinishScope |
net6.0 | 482ns | 0.515ns | 1.99ns | 0.00978 | 0 | 0 | 696 B |
| #5898 | StartFinishScope |
netcoreapp3.1 | 766ns | 0.409ns | 1.47ns | 0.00933 | 0 | 0 | 696 B |
| #5898 | StartFinishScope |
net472 | 884ns | 0.945ns | 3.66ns | 0.104 | 0 | 0 | 658 B |
Benchmarks.Trace.TraceAnnotationsBenchmark - Slower ⚠️ Same allocations ✔️
Slower ⚠️ in #5898
Benchmark
diff/base
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0
1.283
594.61
763.02
| Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0 | 1.283 | 594.61 | 763.02 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | RunOnMethodBegin |
net6.0 | 595ns | 0.204ns | 0.765ns | 0.00967 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
netcoreapp3.1 | 966ns | 0.499ns | 1.93ns | 0.00923 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
net472 | 1.11μs | 0.454ns | 1.57ns | 0.104 | 0 | 0 | 658 B |
| #5898 | RunOnMethodBegin |
net6.0 | 763ns | 0.486ns | 1.88ns | 0.00972 | 0 | 0 | 696 B |
| #5898 | RunOnMethodBegin |
netcoreapp3.1 | 911ns | 1.29ns | 5.01ns | 0.0092 | 0 | 0 | 696 B |
| #5898 | RunOnMethodBegin |
net472 | 1.06μs | 0.501ns | 1.88ns | 0.104 | 0 | 0 | 658 B |
chrisnas
approved these changes
Aug 16, 2024
| @@ -302,7 +291,7 @@ void initLibrary(void) | |||
| if (enableMiniDump != NULL && enableMiniDump[0] == '1') | |||
| { | |||
| // If DOTNET_DbgEnableMiniDump is set, the crash handler should call createdump when done | |||
| char* passthrough = getenv("DD_TRACE_CRASH_HANDLER_PASSTHROUGH"); | |||
| char* passthrough = getenv("DD_INTERNAL_CRASHTRACKING_PASSTHROUGH"); | |||
There was a problem hiding this comment.
Use constants for env var names to avoid repeating them and maybe introduce typos
There was a problem hiding this comment.
Done in 904b7eb
| } | ||
|
|
||
| [TestAppFact("Samples.ExceptionGenerator")] | ||
| public void DontRedirectCrashHandlerIfPathNotSet(string appName, string framework, string appAssembly) |
There was a problem hiding this comment.
Maybe keep this test and rename it into GenerateDumpIfDbgRequested to validate that existing CLR behavior is still working with CrashTracking
There was a problem hiding this comment.
Done in 904b7eb
andrewlock
pushed a commit
that referenced
this pull request
Aug 23, 2024
Normalize the environment variable names used by crashtracking. `DD_CRASHTRACKING_*`: environment variables expected to be set by the customers. Currently: - `DD_CRASHTRACKING_ENABLED`: enables or disables crashtracking (default: enabled) `DD_INTERNAL_CRASHTRACKING_*`: environment variables used by the tracer infrastructure and/or tests. Currently: - `DD_INTERNAL_CRASHTRACKING_PASSTHROUGH`: automatically set to decide whether the real createdump should be called or not - `DD_INTERNAL_CRASHTRACKING_OUTPUT`: save the crash report to a file instead of using telemetry Now that other languages are implementing the feature, it's important to have consistent names. `DD_TRACE_CRASH_HANDLER` is not needed anymore, removed it. Removed the profiler tests that relied on `DD_TRACE_CRASH_HANDLER` (they're not needed now that we have crashtracking tests). The existing tests were updated.
andrewlock
added a commit
that referenced
this pull request
Aug 27, 2024
…-> v2) (#5936) ## Summary of changes Normalize the environment variable names used by crashtracking. `DD_CRASHTRACKING_*`: environment variables expected to be set by the customers. Currently: - `DD_CRASHTRACKING_ENABLED`: enables or disables crashtracking (default: enabled) `DD_INTERNAL_CRASHTRACKING_*`: environment variables used by the tracer infrastructure and/or tests. Currently: - `DD_INTERNAL_CRASHTRACKING_PASSTHROUGH`: automatically set to decide whether the real createdump should be called or not - `DD_INTERNAL_CRASHTRACKING_OUTPUT`: save the crash report to a file instead of using telemetry ## Reason for change Now that other languages are implementing the feature, it's important to have consistent names. ## Implementation details `DD_TRACE_CRASH_HANDLER` is not needed anymore, removed it. Removed the profiler tests that relied on `DD_TRACE_CRASH_HANDLER` (they're not needed now that we have crashtracking tests). ## Test coverage The existing tests were updated. ## Other Details Backport of - #5898 --------- Co-authored-by: Kevin Gosse <kevin.gosse@datadoghq.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
Normalize the environment variable names used by crashtracking.
DD_CRASHTRACKING_*: environment variables expected to be set by the customers. Currently:DD_CRASHTRACKING_ENABLED: enables or disables crashtracking (default: enabled)DD_INTERNAL_CRASHTRACKING_*: environment variables used by the tracer infrastructure and/or tests. Currently:DD_INTERNAL_CRASHTRACKING_PASSTHROUGH: automatically set to decide whether the real createdump should be called or notDD_INTERNAL_CRASHTRACKING_OUTPUT: save the crash report to a file instead of using telemetryReason for change
Now that other languages are implementing the feature, it's important to have consistent names.
Implementation details
DD_TRACE_CRASH_HANDLERis not needed anymore, removed it.Removed the profiler tests that relied on
DD_TRACE_CRASH_HANDLER(they're not needed now that we have crashtracking tests).Test coverage
The existing tests were updated.