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

refactor!: replace SpanAttributes, MetricsAttributes and ResourceAttributes with Attributes #4928

Conversation

david-luna
Copy link
Contributor

@david-luna david-luna commented Aug 19, 2024

Which problem is this PR solving?

Closes: #4175

Fixes # (issue)

Short description of the changes

  • replace SpanAttributes, MetricsAttributes and ResourceAttributes with Attributes
  • replace SpanAttibuteValue and MetricAttributeValue with AttributeValue

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • compile all modules to check types are correct
  • ran unit tests

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@david-luna david-luna changed the title refactor: replace SpanAttributes and MetricsAttributes with Attributes refactor: replace SpanAttributes, MetricsAttributes and ResourceAttributes with Attributes Aug 19, 2024
@david-luna david-luna marked this pull request as ready for review August 19, 2024 15:31
@david-luna david-luna requested a review from a team August 19, 2024 15:31
@david-luna david-luna changed the title refactor: replace SpanAttributes, MetricsAttributes and ResourceAttributes with Attributes refactor!: replace SpanAttributes, MetricsAttributes and ResourceAttributes with Attributes Aug 19, 2024
@pichlermarc pichlermarc added the target:next-major-release This PR targets the next major release (`next` branch) label Aug 20, 2024
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.

Thanks 🎉

The Attribute type was added 1.3.0, so we need to update all minimum @opentelemetry/api versions to at least this 🙂

@david-luna
Copy link
Contributor Author

Thanks 🎉

The Attribute type was added 1.3.0, so we need to update all minimum @opentelemetry/api versions to at least this 🙂

Done!

@david-luna david-luna requested a review from pichlermarc August 21, 2024 14:57
Copy link

codecov bot commented Aug 21, 2024

Codecov Report

Attention: Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.04%. Comparing base (1041ed4) to head (00e7558).
Report is 62 commits behind head on next.

Files Patch % Lines
api/src/metrics/NoopMeter.ts 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4928      +/-   ##
==========================================
- Coverage   92.97%   91.04%   -1.93%     
==========================================
  Files         289       89     -200     
  Lines        7797     1955    -5842     
  Branches     1602      416    -1186     
==========================================
- Hits         7249     1780    -5469     
+ Misses        548      175     -373     
Files Coverage Δ
api/src/metrics/Metric.ts 100.00% <ø> (ø)
api/src/trace/NonRecordingSpan.ts 95.65% <100.00%> (ø)
api/src/trace/SamplingResult.ts 100.00% <ø> (ø)
packages/opentelemetry-resources/src/Resource.ts 100.00% <100.00%> (ø)
...try-resources/src/detectors/BrowserDetectorSync.ts 94.44% <100.00%> (-5.56%) ⬇️
...lemetry-resources/src/detectors/EnvDetectorSync.ts 94.11% <100.00%> (ø)
...ckages/opentelemetry-sdk-trace-base/src/Sampler.ts 100.00% <ø> (ø)
packages/opentelemetry-sdk-trace-base/src/Span.ts 97.59% <100.00%> (ø)
...y-sdk-trace-base/src/sampler/ParentBasedSampler.ts 83.87% <ø> (ø)
api/src/metrics/NoopMeter.ts 95.34% <75.00%> (ø)

... and 201 files with indirect coverage changes

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.

Ah, we actually must not drop the types form the API package, we can just cease to use them in the SDK packages. Only SDK packages will be bumped to 2.0, not the API.

Copy link
Member

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

Whew, this is a doozy. Thanks for all the work on this @david-luna.

Question: Are we sure we want all of these in a single PR? Some things are easier to get done in one shot, but I feel like it might be better to split at least into groups of breaking v nonbreaking, because the nonbreaking ones can happen in main instead of just next.

@david-luna
Copy link
Contributor Author

Whew, this is a doozy. Thanks for all the work on this @david-luna.

Question: Are we sure we want all of these in a single PR? Some things are easier to get done in one shot, but I feel like it might be better to split at least into groups of breaking v nonbreaking, because the nonbreaking ones can happen in main instead of just next.

thanks @JamieDanielson for your feedback

I made the assumption the next branch will be merged to main in a merge commit rather than a rebase so commit history would not matter much.

  • what is going to be the strategy: merge into main, rebase on top of main, ...?
  • do we want these changes already in main if possible knowing that most of them may be breaking?

cc: @pichlermarc, @dyladan

@pichlermarc
Copy link
Member

pichlermarc commented Sep 11, 2024

Whew, this is a doozy. Thanks for all the work on this @david-luna.
Question: Are we sure we want all of these in a single PR? Some things are easier to get done in one shot, but I feel like it might be better to split at least into groups of breaking v nonbreaking, because the nonbreaking ones can happen in main instead of just next.

thanks @JamieDanielson for your feedback

I made the assumption the next branch will be merged to main in a merge commit rather than a rebase so commit history would not matter much.

  • what is going to be the strategy: merge into main, rebase on top of main, ...?

We will merge next into main eventually.

  • do we want these changes already in main if possible knowing that most of them may be breaking?

All changes in this PR that target files in examples/, experimental/packages/, integration-tests/, selenium-tests/ can actually be made against main, since minor version bumps may be breaking if a package is at 0.x.

The benefit of doing that is this: We periodically have to merge main into next until we're ready to merge next into main (and release 2.0) as we have to sync any bugfixes/features from main into the next branch. Now, the more changes we can already make on main, the easier to process of syncing these bugfixes/features from main to next is going to be as there will be less merge-conflicts. 🙂

Once we're then ready to release 2.0, we'll merge next into main, make final adjustments on main and release from there.

@david-luna
Copy link
Contributor Author

david-luna commented Sep 13, 2024

@JamieDanielson @pichlermarc thanks for the feedback.

If I understood well I'll split this one in smaller PRs targeting main. If you don't mind I'll try to group changes just to avoid to have too many PRs (one per package).

Thanks again

@david-luna david-luna closed this Sep 13, 2024
@david-luna david-luna deleted the 4175-replace-span-metrics-resource-attributes branch September 23, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
target:next-major-release This PR targets the next major release (`next` branch)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants