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.

Sign up for GitHub

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

Jump to bottom

Sync profiling version with tracer; append git sha1 to non-release builds #1992

Merged
merged 17 commits into from
Apr 14, 2023
Merged

Sync profiling version with tracer; append git sha1 to non-release builds #1992

merged 17 commits into from
Apr 14, 2023

Conversation

morrisonlevi
Copy link
Collaborator

@morrisonlevi morrisonlevi commented Mar 28, 2023 •
edited

Description

  • The profiling version will now be synchronized with the tracer version.
  • The tracer version on master will be the latest feature release instead of 1.0.0-nightly.
  • Non-release builds will append '+' followed by the git sha1 to the version number.
  • Bump rust version in the Cargo.toml file. We had previously already bumped the version in other places and just missed this one.

Readiness checklist

  • Changelog has been added to the release document.
  • Tests added for this feature/bug.

Reviewer checklist

  • Appropriate labels assigned.
  • Milestone is set.

@morrisonlevi morrisonlevi added the profiling Relates to the Continuous Profiler label Mar 28, 2023
@morrisonlevi morrisonlevi added this to the 0.87.0 milestone Mar 28, 2023
@morrisonlevi morrisonlevi requested a review from a team as a code owner March 28, 2023 13:38
bwoebi
bwoebi reviewed Apr 11, 2023
View reviewed changes
profiling/Cargo.toml Outdated
@@ -1,9 +1,9 @@
[package]
name = "datadog-php-profiling"
version = "0.15.0"
version = "0.87.0-alpha.1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it necessary to keep master on a version number, which needs to regularly updated? I'd strongly prefer having something like 1.0.0-nightly here on master, if possible.
It's not like we're distributing this via cargo anyway...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Customers sometimes run builds from master. I would prefer them to have a real but pre-release version number than being 1.0.0-nightly all the time.

Copy link
Collaborator

@bwoebi bwoebi Apr 12, 2023 •
edited

Choose a reason for hiding this comment

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

@morrisonlevi Would it be possible to instead insert the version in the circleci checkout step (for tracer as well then, replacing it in the 3 relevant files via sed (last version plus 1 appended by the git hash, essentially, giving concrete information about the precise version installed)), or are we talking rather about manual builds from source?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We discussed in person to change our release process so that the version on master is the latest feature release. We'll append +$gitsha e.g. 0.86.0+a885854666d58b3e06792856f8d32f0a2459bafb in CI when the branch doesn't start with ddtrace-. For PECL we'll have to replace + with - or just strip it. These PECL builds aren't released anyway, so it doesn't exactly matter, but it will choke on +.

@morrisonlevi
Sync profiling version number with tracer
ec8ae6a 
Also bump rust version in the Cargo.toml file. We had previously
already bumped the version in other places and just missed this one.
@morrisonlevi
Bump version to 0.86.3 on master
525d81c 
This is part of a change in our release process. Further changes will
be made to append a '+' symbol followed by the build hash.
@morrisonlevi
Adjust downstream gitlab branch
19dd778
@morrisonlevi morrisonlevi requested a review from a team as a code owner April 12, 2023 17:24
@morrisonlevi morrisonlevi changed the title Sync profiling version number with tracer Sync profiling version number with tracer; add gitsha1 to version number Apr 12, 2023
@morrisonlevi morrisonlevi changed the title Sync profiling version number with tracer; add gitsha1 to version number Sync profiling version with tracer; append gitsha1 to non-release builds Apr 12, 2023
@morrisonlevi morrisonlevi changed the title Sync profiling version with tracer; append gitsha1 to non-release builds Sync profiling version with tracer; append git sha1 to non-release builds Apr 12, 2023
morrisonlevi
morrisonlevi commented Apr 13, 2023
View reviewed changes
tooling/bin/pecl-build Outdated Show resolved Hide resolved
bwoebi
bwoebi approved these changes Apr 13, 2023
View reviewed changes
Copy link
Collaborator

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

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

Generally looks nice to me now :-)

@morrisonlevi
Copy link
Collaborator Author

morrisonlevi commented Apr 14, 2023 •
edited

Installing from php datadog-setup.php --enable-profiling --php-bin all didn't work. Actually, it did, and GitHub is linking to the wrong package extension job from this PR. This is concerning, but if I navigate to the right place in CircleCI, it does work. Artifacts are named like dd-library-php-0.86.3+9ee8700c545e5412ecacb16e06b9dd74ba613ddf-aarch64-linux-gnu.tar.gz, and download and install correctly.

The package extension jobs for the faux-release branch ddtrace-0.86.3.2 looks good and it deployed to the relenv, though.

r1viollet
r1viollet reviewed Apr 14, 2023
View reviewed changes
.circleci/config.yml
if [[ "$CIRCLE_BRANCH" =~ "ddtrace-" ]] ; then
echo "Release branch detected; not adding git sha1 to version number."
else
sed -E -i "s/const\s+VERSION\s*=\s*'(.*)'/const VERSION = '\1+$githash'/g" "src/DDTrace/Tracer.php"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Considering this modifies sources, can we easily make it part of the build process ? Like a different makefile target ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

we talked about potential improvements to unify the version management across languages (rust / php / C) which would involve having a make target to propagate version numbers.
Accepting PR for now as this won't change the result of the build.

r1viollet
r1viollet reviewed Apr 14, 2023
View reviewed changes
.gitlab-ci.yml Outdated Show resolved Hide resolved
r1viollet
r1viollet approved these changes Apr 14, 2023
View reviewed changes
Copy link
Collaborator

@r1viollet r1viollet left a comment

Choose a reason for hiding this comment

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

LTGM

@morrisonlevi morrisonlevi merged commit 2a31ebb into master Apr 14, 2023
@morrisonlevi morrisonlevi deleted the levi/sync-version branch April 14, 2023 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwoebi bwoebi bwoebi approved these changes

@r1viollet r1viollet r1viollet approved these changes

Assignees
No one assigned
Labels
ci profiling Relates to the Continuous Profiler
Projects
None yet
Milestone
0.87.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@morrisonlevi @bwoebi @r1viollet