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

Bump rust to 1.78 and add feature to Cargo.toml according to libdatadog changes #3005

Merged
merged 6 commits into from
Jan 6, 2025

Conversation

dubloom
Copy link
Contributor

@dubloom dubloom commented Dec 19, 2024

Description

libdatadog removed the set_cgroup_file feature. However it is used in some dd-trace-php tests. As we are not sure they can be removed and without any changes the tracer cannot be built, they added back but under a feature flag.

This PR activates the feature flag and therefore enable the tracer to be built again.

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@dubloom dubloom requested a review from a team as a code owner December 19, 2024 09:33
@dubloom dubloom requested a review from iamluc December 19, 2024 09:34
@iamluc
Copy link
Contributor

iamluc commented Dec 20, 2024

@dubloom You probably have to update the reference of the libdatadog submodule to make it work

@dubloom dubloom requested a review from a team as a code owner December 20, 2024 12:43
@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 38.62%. Comparing base (205c6d8) to head (39df479).

❗ There is a different number of reports uploaded between BASE (205c6d8) and HEAD (39df479). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (205c6d8) HEAD (39df479)
tracer-php 11 9
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #3005       +/-   ##
=============================================
- Coverage     74.80%   38.62%   -36.19%     
  Complexity     2781     2781               
=============================================
  Files           112      112               
  Lines         11017    11017               
=============================================
- Hits           8241     4255     -3986     
- Misses         2776     6762     +3986     
Flag Coverage Δ
tracer-php 38.62% <ø> (-36.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 42 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 205c6d8...39df479. Read the comment docs.

@pr-commenter
Copy link

pr-commenter bot commented Dec 20, 2024

Benchmarks [ tracer ]

Benchmark execution time: 2024-12-20 13:11:47

Comparing candidate commit aae29fb in PR branch dubloom/add-cargo-feature-flag with baseline commit 0d57b55 in branch master.

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

scenario:MessagePackSerializationBench/benchMessagePackSerialization

  • 🟥 execution_time [+6.135µs; +8.285µs] or [+3.604%; +4.867%]

scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache

  • 🟥 execution_time [+6.531µs; +8.309µs] or [+3.868%; +4.921%]

scenario:PDOBench/benchPDOBaseline

  • 🟥 execution_time [+10.940µs; +15.320µs] or [+6.014%; +8.421%]

@@ -8,7 +8,7 @@ crate-type = ["staticlib", "cdylib"]
path = "lib.rs"

[dependencies]
ddcommon = { path = "../libdatadog/ddcommon" }
ddcommon = { path = "../libdatadog/ddcommon", features = ["cgroup_testing"] }
Copy link

Choose a reason for hiding this comment

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

If this is for tests, you probably don't want this feature as a regular dependency. You can leave the ddcommon dependency as it was here, and under dev-dependencies add it again with this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i might do it wrong, but when i try, themake install command will fail because set_cgroup_file is not found.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's for tests, but for PHP extension tests. Which are (also) run on release builds.

@bwoebi bwoebi requested a review from a team as a code owner January 2, 2025 20:06
@bwoebi bwoebi force-pushed the dubloom/add-cargo-feature-flag branch 4 times, most recently from e880898 to 5b1d482 Compare January 3, 2025 14:25
@pr-commenter
Copy link

pr-commenter bot commented Jan 3, 2025

Benchmarks [ profiler ]

Benchmark execution time: 2025-01-06 16:32:50

Comparing candidate commit 39df479 in PR branch dubloom/add-cargo-feature-flag with baseline commit 205c6d8 in branch master.

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

scenario:walk_stack/50

  • 🟥 wall_time [+723.415ns; +732.872ns] or [+4.989%; +5.054%]

scenario:walk_stack/99

  • 🟥 wall_time [+725.946ns; +730.181ns] or [+5.034%; +5.063%]

@bwoebi bwoebi changed the title refactor: add feature_flag to Cargo.toml according to libdatadog changes Bump rust to 1.78 and add feature to Cargo.toml according to libdatadog changes Jan 3, 2025
@bwoebi bwoebi force-pushed the dubloom/add-cargo-feature-flag branch from 5b1d482 to 2488f64 Compare January 3, 2025 23:41
@bwoebi bwoebi force-pushed the dubloom/add-cargo-feature-flag branch from 918efa2 to dd16141 Compare January 6, 2025 15:35
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
Add SSRF Rasp capability (#814)
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
@bwoebi bwoebi force-pushed the dubloom/add-cargo-feature-flag branch from dd16141 to 39df479 Compare January 6, 2025 16:21
@bwoebi bwoebi merged commit 94538cb into master Jan 6, 2025
490 of 518 checks passed
@bwoebi bwoebi deleted the dubloom/add-cargo-feature-flag branch January 6, 2025 17:59
Copy link
Collaborator

@morrisonlevi morrisonlevi left a comment

Choose a reason for hiding this comment

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

Approved on the profiling side 👍🏻

@github-actions github-actions bot added this to the 1.6.0 milestone Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants