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

chore(semconv): Separate SemConv releases to allow different versioning #4904

Conversation

JamieDanielson
Copy link
Member

@JamieDanielson JamieDanielson commented Aug 7, 2024

Which problem is this PR solving?

Sometimes folks get confused about the version of this package, expecting it to match the version of semantic conventions being produced. This becomes especially problematic as the package version number is now very close to the semantic convention version number. By releasing separately we can make the versions match the expectation.

Fixes #4878

Short description of the changes

  • add new workspace for semconv
  • add separate changelog for semconv
  • update release scripts and workflow to separate semconv
  • update contributing guides to clarify semconv is separate
  • update folder name from opentelemetry-semantic-conventions to semantic-conventions
  • move semantic-conventions outside of packages directory
  • update all paths and references to the old name

This looks like a really big change, but it's mostly renaming files and updating paths.

Type of change

  • This change requires a documentation update

How Has This Been Tested?

To test release notes script:

  • replace Unreleased in the CHANGELOG to an actual version, like 1.1.1. Then run the script as described in the comments (node scripts/extract-latest-release-notes.js ./semantic-conventions/CHANGELOG.md). There should be a .tmp/release-notes.md with the contents of the changelog for that version.

Also tested in separate repo to confirm successful separation of release PRs

Checklist:

  • Followed the style guidelines of this project
  • Documentation has been updated

@JamieDanielson JamieDanielson requested a review from a team August 7, 2024 20:26
@pichlermarc
Copy link
Member

pichlermarc commented Aug 8, 2024

Note: This seemed extra complex - and is likely much more fragile - because it is still in the packages/ directory. I think this should be in a separate directory to simplify things. Wanted to get feedback before moving it though.

I think moving it to its own directory is a good idea. 👍 It would also be a good occasion to remove the opentelemetry- prefix.

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

I tried running the workflow and noticed that scripts/extract-latest-release-notes.js does not correctly handle the case of a new CHANGELOG.md file without a previous version.

We may need to change it like so to also handle this case:

--- a/scripts/extract-latest-release-notes.js
+++ b/scripts/extract-latest-release-notes.js
@@ -16,7 +16,7 @@ function extractLatestChangelog(changelogPath) {
   const changelog = fs.readFileSync(changelogPath).toString();
   // Matches everything from the first entry at h2 ('##') followed by a space and a non-prerelease semver version
   // until the next entry at h2.
-  const firstReleaseNoteEntryExp = /^## \d+\.\d+\.\d\n.*?(?=^## )/ms;
+  const firstReleaseNoteEntryExp = /^## \d+\.\d+\.\d\n.*?((?=^## )|$(?![\r\n]))/ms;
 
   return changelog.match(firstReleaseNoteEntryExp)[0];
 }

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated
Comment on lines 46 to 47
"prepare_release:semconv:patch": "npm run _check:no_changes && npm run _backup:package-json && npm run _lerna:remove_api && npm run _lerna:remove_stable_except_semconv && npm run _lerna:remove_experimental && npm run _lerna:version_patch && npm run _restore:package-json && npm run _changelog:prepare_semconv",
"prepare_release:semconv:minor": "npm run _check:no_changes && npm run _backup:package-json && npm run _lerna:remove_api && npm run _lerna:remove_stable_except_semconv && npm run _lerna:remove_experimental && npm run _lerna:version_minor && npm run _restore:package-json && npm run _changelog:prepare_semconv",
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, both of these will also bump examples and such, so they will diverge over time from the experimental/stable versioning over time. Maybe we should also exclude them from the update. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean we want to ensure absolutely nothing is included in this EXCEPT for the semantic-conventions package, right? Can you look here if this matches what you're suggesting: 1d7a1bd

@pichlermarc
Copy link
Member

Not sure if this question has been answered already: if we ever have to bump major for @opentelemetry/semantic-conventions, we'll be out of sync again. My guess is that we don't want to include this package the upcoming 2.0 bump then, right?

@trentm
Copy link
Contributor

trentm commented Aug 9, 2024

-  const firstReleaseNoteEntryExp = /^## \d+\.\d+\.\d\n.*?(?=^## )/ms;
+  const firstReleaseNoteEntryExp = /^## \d+\.\d+\.\d\n.*?((?=^## )|$(?![\r\n]))/ms;
 

Nice trick. Python's regex syntax has \Z (https://docs.python.org/3/library/re.html#index-36) for this: to match the end of the string when using multiline mode (the m) modifier.

Another option would be to append a fake ## section to the end of the changelog before matching:

const firstReleaseNoteEntryExp = /^## \d+\.\d+\.\d\n.*?(?=^## )/ms;   // same as before

return (changelog + '\n## eof').match(firstReleaseNoteEntryExp)[0];

/regex-nerd>

The regex checks for existence of the previous h2
but new packages will not have versions before it.
Add a dummy h2 to satisfy the regex with minimal
overall changes.
To test this, replace Unreleased in the CHANGELOG
to an actual version, like 1.1.1.
Then run the script as described in the comments.
There should be a .tmp/release-notes.md with
the contents of the changelog for that version.
either way works fine - using the dummy h2 entry
or this updated regex. the benefit of this regex
change is that it still matches the intent of the
variable being used. shrug.
@JamieDanielson JamieDanielson force-pushed the jamie.separate-semconv-release-publishing branch from 1439c46 to e1e0cf8 Compare August 13, 2024 22:02
@JamieDanielson
Copy link
Member Author

Not sure if this question has been answered already: if we ever have to bump major for @opentelemetry/semantic-conventions, we'll be out of sync again. My guess is that we don't want to include this package the upcoming 2.0 bump then, right?

Right, we'll want to keep this versioned independently as much as possible. The changes made on #4690 contain the individually exported strings and newly prefixed attributes, so this should be ok for a long while without needing any kind of major version bump.

There may be a day that we have this package in its own repo, similar to how Java has a separate repo for semantic conventions. Today we're just keeping it in a separate release flow.

@JamieDanielson
Copy link
Member Author

The browser tests are failing because of rate limiting. Hopefully it's not too much from this PR (there are lots of commits but most were done locally in batches before pushing)

Copy link

codecov bot commented Aug 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.62%. Comparing base (ecc88a3) to head (e1e0cf8).
Report is 82 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4904      +/-   ##
==========================================
+ Coverage   91.04%   91.62%   +0.58%     
==========================================
  Files          89      146      +57     
  Lines        1954     3153    +1199     
  Branches      416      682     +266     
==========================================
+ Hits         1779     2889    +1110     
- Misses        175      264      +89     

see 65 files with indirect coverage changes

@JamieDanielson
Copy link
Member Author

JamieDanielson commented Aug 15, 2024

Just an update, I've confirmed in a separate testing repo that these will release separately with a few more changes. A big piece of that is that the whole semconv package needed its own directory, outside of the packages directory. This makes sense since it's versioned separately anyway, and isn't part of the "sdk" release scope that includes stable and experimental packages.
I'll move those changes into this PR and ping for re-review.

- experimental # all packages in experimental/packages
- sdk # all SDK packages, experimental and stable, excluding semantic conventions
- semconv # only semantic convention package
- all # all release packages, including API, excluding semconv
Copy link
Member Author

Choose a reason for hiding this comment

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

Added clarity for what's included for each option. "All" is now somewhat misleading since semconv isn't in there, but I figured that would make this big change even bigger and figured we could change "all" to something else later. Semconv should never be released on a schedule with anything else other than semconv versioning.

@JamieDanielson
Copy link
Member Author

Changes made, this should be ready for re-review!

Copy link
Contributor

@trentm trentm left a comment

Choose a reason for hiding this comment

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

Looking good.

doc/contributing/releasing.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated
"_github:draft_release:all": "npm run _github:draft_release:api && _github:draft_release:experimental && _github:draft_release:stable",
"_github:draft_release:api": "node scripts/extract-latest-release-notes.js ./api/CHANGELOG.md && VERSION=$(node scripts/get-version.js ./api/package.json | grep -oP '^\\d+\\.\\d+\\.\\d+$') && gh release create --draft --notes-file ./.tmp/release-notes.md --target main --title api/v$VERSION api/v$VERSION",
"_github:draft_release:experimental": "node scripts/extract-latest-release-notes.js ./experimental/CHANGELOG.md && VERSION=$(node scripts/get-version.js ./experimental/packages/ | grep -oP '^\\d+\\.\\d+\\.\\d+$') && gh release create --draft --notes-file ./.tmp/release-notes.md --target main --title experimental/v$VERSION experimental/v$VERSION",
"_github:draft_release:stable": "node scripts/extract-latest-release-notes.js ./CHANGELOG.md && VERSION=$(node scripts/get-version.js ./packages/ | grep -oP '^\\d+\\.\\d+\\.\\d+$') && gh release create --draft --notes-file ./.tmp/release-notes.md --target main --title v$VERSION v$VERSION",
"_github:draft_release:semconv": "node scripts/extract-latest-release-notes.js ./semantic-conventions/CHANGELOG.md && VERSION=$(node scripts/get-version.js ./semantic-conventions/package.json | grep -oP '^\\d+\\.\\d+\\.\\d+$') && gh release create --draft --notes-file ./.tmp/release-notes.md --target main --title semconv/v$VERSION semconv/v$VERSION",
Copy link
Contributor

Choose a reason for hiding this comment

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

(Note: This is no different than the existing _github:draft_release:* scripts.)
That -P option to grep doesn't exist on macOS' grep. So the Create GitHub Releases manual step will fail if you are running on macOS. @pichlermarc uses Linux, so it'll work for him. ;)

This works with grep -oE ... on macOS where -E is Interpret pattern as an extended regular expression

% node scripts/get-version.js ./semantic-conventions/package.json | grep -Eo '^\d+\.\d+\.\d+$'
1.25.1
% echo $?
0

% node scripts/get-version.js ./semantic-conventions/package.json | grep -o '^\d+\.\d+\.\d+$'
% echo $?
1

-E works on alpine:

% docker run --rm -ti alpine /bin/sh
/ # echo '1.25.1' | grep -o '^\d+\.\d+\.\d+$'
/ # echo '1.25.1' | grep -oE '^\d+\.\d+\.\d+$'
1.25.1

@pichlermarc Would -E (instead of -P) work for usage on your machine?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, yes GNU grep vs. FreeBSD grep vs. Busybox grep. 😓
With GNU grep, echo '1.25.1' | grep -oE '^\d+\.\d+\.\d+$' does not work, unfortunately, but:

echo '1.25.1' | grep -oE '^[[:digit:]]+\.[[:digit:]]+\.[[:digit:]]+$' does work.

Also works in alpine (busybox) - if it also works on macOS then we can switch to that 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! This does work on mac, I'll make that change

➜  node scripts/get-version.js ./semantic-conventions/package.json | grep -oE '^[[:digit:]]+\.[[:digit:]]+\.[[:digit:]]+$'
1.25.1
➜  echo $?
0

Copy link
Member Author

@JamieDanielson JamieDanielson Aug 19, 2024

Choose a reason for hiding this comment

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

I am making a similar change on the _verify_release_kind step as well. That is only the -oE change, not a bunch of regex, so it seems like it should work there too.

Copy link
Contributor

Choose a reason for hiding this comment

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

[[:digit:]] could likely be the shorter [0-9] if we want, FWIW.

Also "busybox grep" vs "alpine grep" is scaring me a bit. One works, one doesn't: :)

% docker run --rm -ti busybox
/ # echo '1.25.1' | grep -o '^\d+\.\d+\.\d+$'
/ # echo '1.25.1' | grep -oE '^\d+\.\d+\.\d+$'
/ #

% docker run --rm -ti alpine
/ # echo '1.25.1' | grep -o '^\d+\.\d+\.\d+$'
/ # echo '1.25.1' | grep -oE '^\d+\.\d+\.\d+$'
1.25.1

At least for the versions of those images I have locally.

Ah, that is an issue with \d vs the [[:digit:]] character class again. It works with [[:digit:]] and [0-9]:

% docker run --rm -ti busybox
/ # echo '1.25.1' | grep -oE '^[[:digit:]]+\.[[:digit:]]+\.[[:digit:]]+$'
1.25.1
/ # echo '1.25.1' | grep -oE '^[0-9]+\.[0-9]+\.[0-9]+$'
1.25.1

c.f. https://xkcd.com/1168/

Copy link
Member

@pichlermarc pichlermarc Aug 20, 2024

Choose a reason for hiding this comment

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

[[:digit:]] could likely be the shorter [0-9] if we want, FWIW.

ah yes - I did not really think about length of what I'm writing so that may be a better option. I was just hunting for a \d replacement that I missed the obvious option of [0-9]. 🤦

c.f. https://xkcd.com/1168/

😂

Copy link
Member Author

Choose a reason for hiding this comment

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

xkcd has so many gems 🤣

I've replaced [[:digit:]] with [0-9] and confirmed it worked on my mac.

echo '1.25.1' | grep -oE '^[0-9]+\.[0-9]+\.[0-9]+$'
1.25.1

semantic-conventions/CHANGELOG.md Outdated Show resolved Hide resolved
@pichlermarc
Copy link
Member

-  const firstReleaseNoteEntryExp = /^## \d+\.\d+\.\d\n.*?(?=^## )/ms;
+  const firstReleaseNoteEntryExp = /^## \d+\.\d+\.\d\n.*?((?=^## )|$(?![\r\n]))/ms;
 

Nice trick. Python's regex syntax has \Z (https://docs.python.org/3/library/re.html#index-36) for this: to match the end of the string when using multiline mode (the m) modifier.

Ah - I should've mentioned earlier that I took this from a StackOverflow answer (https://stackoverflow.com/a/34958727), so this is not my doing actually. 🙂

package.json Outdated Show resolved Hide resolved
package.json Outdated
"_github:draft_release:all": "npm run _github:draft_release:api && _github:draft_release:experimental && _github:draft_release:stable",
"_github:draft_release:api": "node scripts/extract-latest-release-notes.js ./api/CHANGELOG.md && VERSION=$(node scripts/get-version.js ./api/package.json | grep -oP '^\\d+\\.\\d+\\.\\d+$') && gh release create --draft --notes-file ./.tmp/release-notes.md --target main --title api/v$VERSION api/v$VERSION",
"_github:draft_release:experimental": "node scripts/extract-latest-release-notes.js ./experimental/CHANGELOG.md && VERSION=$(node scripts/get-version.js ./experimental/packages/ | grep -oP '^\\d+\\.\\d+\\.\\d+$') && gh release create --draft --notes-file ./.tmp/release-notes.md --target main --title experimental/v$VERSION experimental/v$VERSION",
"_github:draft_release:stable": "node scripts/extract-latest-release-notes.js ./CHANGELOG.md && VERSION=$(node scripts/get-version.js ./packages/ | grep -oP '^\\d+\\.\\d+\\.\\d+$') && gh release create --draft --notes-file ./.tmp/release-notes.md --target main --title v$VERSION v$VERSION",
"_github:draft_release:semconv": "node scripts/extract-latest-release-notes.js ./semantic-conventions/CHANGELOG.md && VERSION=$(node scripts/get-version.js ./semantic-conventions/package.json | grep -oP '^\\d+\\.\\d+\\.\\d+$') && gh release create --draft --notes-file ./.tmp/release-notes.md --target main --title semconv/v$VERSION semconv/v$VERSION",
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, yes GNU grep vs. FreeBSD grep vs. Busybox grep. 😓
With GNU grep, echo '1.25.1' | grep -oE '^\d+\.\d+\.\d+$' does not work, unfortunately, but:

echo '1.25.1' | grep -oE '^[[:digit:]]+\.[[:digit:]]+\.[[:digit:]]+$' does work.

Also works in alpine (busybox) - if it also works on macOS then we can switch to that 🙂

scripts/extract-latest-release-notes.js Show resolved Hide resolved
@JamieDanielson
Copy link
Member Author

@trentm @pichlermarc thank you for the reviews! I've incorporated all the feedback suggested here, and resolved all conversations except the one about grepping based on regex, just to highlight the additional change to the verify_release_kind script. Then I think this will be good to go.

I guess we will be a version behind anyway since the semconv package version is at 1.25.1 and we want to get it to 1.27, so we may get a free test-run on the first publish under 1.26 😄

@trentm
Copy link
Contributor

trentm commented Aug 20, 2024

the semconv package version is at 1.25.1 and we want to get it to 1.27, so we may get a free test-run on the first publish under 1.26

Let's take it as a security blanket. Then if things are working we can do a follow-up release that does little but bump the minor to catch up.

I wonder (separately) if it would be possible to add a guard/lint to the semconv release process to ensure that it breaks if it is about to release a version whose X.Y does not match the corresponding semconv version.

@JamieDanielson JamieDanielson added this pull request to the merge queue Aug 20, 2024
Merged via the queue into open-telemetry:main with commit 583154c Aug 20, 2024
18 checks passed
@JamieDanielson JamieDanielson deleted the jamie.separate-semconv-release-publishing branch August 20, 2024 21:30
trentm added a commit to trentm/opentelemetry-js that referenced this pull request Aug 21, 2024
This is to be used as part of the semconv release process.

Refs: open-telemetry#4904
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
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.

Separate publishing of semantic conventions for more control over version
3 participants