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

2.x prep #1463

Open
wants to merge 16 commits into
base: 2.x
Choose a base branch
from
Open

2.x prep #1463

wants to merge 16 commits into from

Conversation

brettmc
Copy link
Collaborator

@brettmc brettmc commented Dec 23, 2024

Initial work for a 2.x branch:

  • make PHP 8.2 the minimum
  • make 2.x-dev alias
  • bump phpunit dependency
  • run rector over the codebase and implement its recommended 8.2 changes
  • update SPI PackageDependency values

Copy link

codecov bot commented Dec 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 73.23%. Comparing base (b515b6b) to head (397d818).

Files with missing lines Patch % Lines
src/Context/ZendObserverFiber.php 0.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #1463      +/-   ##
============================================
- Coverage     73.27%   73.23%   -0.04%     
+ Complexity     2683     2682       -1     
============================================
  Files           387      387              
  Lines          8014     8014              
============================================
- Hits           5872     5869       -3     
- Misses         2142     2145       +3     
Flag Coverage Δ
8.1 ?
8.2 ?
8.3 ?
8.4 73.20% <0.00%> (-0.02%) ⬇️
8.5 73.20% <0.00%> (ø)

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

Files with missing lines Coverage Δ
src/Context/ZendObserverFiber.php 0.00% <0.00%> (ø)

... and 3 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 b515b6b...397d818. Read the comment docs.

@brettmc brettmc marked this pull request as ready for review December 24, 2024 01:31
@brettmc brettmc requested a review from a team as a code owner December 24, 2024 01:31
@@ -35,7 +34,8 @@
},
"extra": {
"branch-alias": {
"dev-main": "1.1.x-dev"
"dev-main": "1.1.x-dev",
"dev-2.x": "2.x-dev"
Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider keeping the API and Context packages on 1.x; bumping these packages to a new major version requires updating every instrumentation package which might slow down adoption of an SDK 2.x release.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's reasonable. They will still exist in the 2.x branch (as will proto, semconv), but we can avoid releasing a new version until we do make a breaking change. I think the most likely trigger for this would be removing the registry in favour of SPI (and possibly removing globals initializers at the same time).

@@ -4,14 +4,14 @@

namespace OpenTelemetry\SDK\Metrics\Data;

final class Histogram implements DataInterface
final readonly class Histogram implements DataInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I don't like changing readonly properties to readonly classes automatically as this might result in unnecessary diffs if we decide to change a property back to non-readonly / decide to add a non-readonly property in the future1. I would prefer if we apply this change only to classes that should truly be readonly.

Footnotes

  1. e.g. the DataInterface implementations have mutable $dataPoints in the upstream version of the metrics SDK to support MetricFilter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

reverted the rector changes and disabled this rule

@@ -26,7 +26,8 @@
},
"extra": {
"branch-alias": {
"dev-main": "1.x-dev"
"dev-main": "1.x-dev",
"dev-2.x": "2.x-dev"
Copy link
Contributor

Choose a reason for hiding this comment

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

The version of this package is based on the https://github.com/open-telemetry/semantic-conventions version? -> should stay on 1.x.

@@ -31,7 +31,8 @@
},
"extra": {
"branch-alias": {
"dev-main": "1.x-dev"
"dev-main": "1.x-dev",
"dev-2.x": "2.x-dev"
Copy link
Contributor

Choose a reason for hiding this comment

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

@brettmc
Copy link
Collaborator Author

brettmc commented Jan 6, 2025

I've pared this PR back:

  • minimum PHP version is bumped to 8.2 for most components (but, we are not obligated to release a 2.x version of anything until we do have a breaking change)
  • aliases, gitsplit config, github workflow config for 2.x
  • package requirements kept at 1.x, until we do have 2.x version(s)

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.

2 participants