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

Fix checking for changes to generated files #6412

Merged
merged 2 commits into from
Dec 9, 2024

Conversation

andrewlock
Copy link
Member

Summary of changes

  • Fixes the verify_source_generated_changes_are_persisted.yml workflow
  • Adds the missing changes

Reason for change

This workflow was broken when we stopped using a source generator to generate the call target integrations, so it was no longer flagging cases where the call target files have not been correctly updated and need to be rebuilt using Nuke

Implementation details

Add the additional paths to include in the diff comparison

Test coverage

Did a manual test locally. Also committing this in 2 parts, to confirm it flags the currently broken master.

Other details

@andrewlock andrewlock added the area:builds project files, build scripts, pipelines, versioning, releases, packages label Dec 9, 2024
@andrewlock andrewlock requested review from a team as code owners December 9, 2024 14:24
@datadog-ddstaging
Copy link

datadog-ddstaging bot commented Dec 9, 2024

Datadog Report

Branch report: andrew/ci/fix-calltarget-checks
Commit report: a51d034
Test service: dd-trace-dotnet

✅ 0 Failed, 470202 Passed, 3685 Skipped, 32h 16m 56.45s Total Time

@andrewlock
Copy link
Member Author

andrewlock commented Dec 9, 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:

  • Welch test with statistical test for significance of 5%
  • Only results indicating a difference greater than 5% and 5 ms are considered.

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

gantt
    title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6412) - mean (69ms)  : 66, 73
     .   : milestone, 69,
    master - mean (70ms)  : 67, 72
     .   : milestone, 70,

    section CallTarget+Inlining+NGEN
    This PR (6412) - mean (980ms)  : 957, 1003
     .   : milestone, 980,
    master - mean (988ms)  : 963, 1014
     .   : milestone, 988,

Loading
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6412) - mean (108ms)  : 106, 111
     .   : milestone, 108,
    master - mean (110ms)  : 107, 114
     .   : milestone, 110,

    section CallTarget+Inlining+NGEN
    This PR (6412) - mean (680ms)  : 666, 694
     .   : milestone, 680,
    master - mean (687ms)  : 669, 704
     .   : milestone, 687,

Loading
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6412) - mean (92ms)  : 90, 94
     .   : milestone, 92,
    master - mean (93ms)  : 91, 96
     .   : milestone, 93,

    section CallTarget+Inlining+NGEN
    This PR (6412) - mean (633ms)  : 617, 650
     .   : milestone, 633,
    master - mean (643ms)  : 626, 660
     .   : milestone, 643,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6412) - mean (191ms)  : 186, 197
     .   : milestone, 191,
    master - mean (190ms)  : 186, 194
     .   : milestone, 190,

    section CallTarget+Inlining+NGEN
    This PR (6412) - mean (1,093ms)  : 1060, 1127
     .   : milestone, 1093,
    master - mean (1,091ms)  : 1063, 1119
     .   : milestone, 1091,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6412) - mean (276ms)  : 272, 281
     .   : milestone, 276,
    master - mean (276ms)  : 272, 280
     .   : milestone, 276,

    section CallTarget+Inlining+NGEN
    This PR (6412) - mean (868ms)  : 839, 897
     .   : milestone, 868,
    master - mean (869ms)  : 839, 900
     .   : milestone, 869,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6412) - mean (266ms)  : 261, 271
     .   : milestone, 266,
    master - mean (266ms)  : 262, 269
     .   : milestone, 266,

    section CallTarget+Inlining+NGEN
    This PR (6412) - mean (848ms)  : 810, 885
     .   : milestone, 848,
    master - mean (850ms)  : 818, 882
     .   : milestone, 850,

Loading

@andrewlock andrewlock force-pushed the andrew/ci/fix-calltarget-checks branch from a20a558 to a51d034 Compare December 9, 2024 16:53
@andrewlock andrewlock merged commit ee9d184 into master Dec 9, 2024
106 of 109 checks passed
@andrewlock andrewlock deleted the andrew/ci/fix-calltarget-checks branch December 9, 2024 18:42
@github-actions github-actions bot added this to the vNext-v3 milestone Dec 9, 2024
veerbia pushed a commit that referenced this pull request Dec 16, 2024
## Summary of changes

- Fixes the `verify_source_generated_changes_are_persisted.yml` workflow
- Adds the missing changes

## Reason for change

This workflow was broken when we stopped using a source generator to
generate the call target integrations, so it was no longer flagging
cases where the call target files have _not_ been correctly updated and
need to be rebuilt using Nuke

## Implementation details

Add the additional paths to include in the diff comparison

## Test coverage

Did a manual test locally. Also committing this in 2 parts, to confirm
it flags the currently broken master.

## Other details

This missing file updates should have been added in #6397 but were missed because this was broken
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:builds project files, build scripts, pipelines, versioning, releases, packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants