Skip to content

Restructure API / SDK components (Attributes, Clock, Trace)#521

Merged
brettmc merged 9 commits into
open-telemetry:mainfrom
Nevay:feature/restructure
Jan 1, 2022
Merged

Restructure API / SDK components (Attributes, Clock, Trace)#521
brettmc merged 9 commits into
open-telemetry:mainfrom
Nevay:feature/restructure

Conversation

@Nevay

@Nevay Nevay commented Dec 24, 2021

Copy link
Copy Markdown
Contributor
  • moves Attributes and Clock from SDK/Trace to SDK, needed by metrics too
  • moves Event and Link from API to SDK, only needed for readable spans, not used by API
  • moves NoopTracer from SDK to API as required by specification
  • moves AttributesInterface from API to SDK by widening types in API to iterable
    previous implementation required API consumers to depend on the SDK implementation to provide attributes to API methods
    bc-breaks:
    • Attribute abstraction is removed, Attributes now return only the value instead of {name,value} pairs
    • iterable $attributes arguments are now non-nullable
    • (todo: rename Attributes::setAttribute() to Attributes::set() to be consistent with Attributes::get() ?)
  • moves ClockInterface from API to SDK
    currently only used in SDK, interface+implementation should either be fully available in API or just in SDK

@codecov

codecov Bot commented Dec 24, 2021 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #521 (bbc4fec) into main (fb00203) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #521      +/-   ##
============================================
+ Coverage     95.81%   95.88%   +0.06%     
+ Complexity      955      934      -21     
============================================
  Files            95       94       -1     
  Lines          2320     2286      -34     
============================================
- Hits           2223     2192      -31     
+ Misses           97       94       -3
Impacted Files Coverage Δ
src/API/Trace/NoopTracer.php 66.66% <ø> (ø)
src/SDK/AttributeLimits.php 100.00% <ø> (ø)
src/SDK/Trace/Sampler/AlwaysOffSampler.php 100.00% <ø> (ø)
src/SDK/Trace/Sampler/AlwaysOnSampler.php 100.00% <ø> (ø)
src/SDK/Trace/Sampler/ParentBased.php 100.00% <ø> (ø)
src/SDK/Trace/Sampler/TraceIdRatioBasedSampler.php 100.00% <ø> (ø)
src/SDK/Trace/SpanLimits.php 100.00% <ø> (ø)
src/SDK/Trace/TracerProvider.php 100.00% <ø> (ø)
src/API/Trace/NonRecordingSpan.php 100.00% <100.00%> (ø)
src/API/Trace/NoopSpanBuilder.php 100.00% <100.00%> (ø)
... and 28 more

Continue to review full report at Codecov.

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

brettmc
brettmc reviewed Dec 24, 2021
View reviewed changes
Comment thread src/SDK/Trace/ImmutableSpan.php
brettmc
brettmc reviewed Dec 24, 2021
View reviewed changes
Comment thread tests/SDK/Util/SpanData.php Outdated
@brettmc

brettmc commented Dec 24, 2021

Copy link
Copy Markdown
Contributor

I've taken a first pass over this - I like! Couple of comments to be looked at, and I'm in favour of renaming setAttribute to set - we're still pre-alpha so breaking things for the better is fine.

tidal
tidal suggested changes Dec 25, 2021
View reviewed changes
Comment thread tests/SDK/Unit/Trace/SpanExporter/FriendlySpanConverterTest.php Outdated
@Nevay Nevay mentioned this pull request Dec 25, 2021
Merged
@tidal

tidal commented Dec 28, 2021

Copy link
Copy Markdown
Member

Thanks for your changes (sorry for the delay, I was busy over the holidays).
There are some merge conflics now, otherwise It's looking good imo.

@Nevay
Nevay force-pushed the feature/restructure branch from e14dc59 to eaa4f53 Compare December 28, 2021 15:59
@brettmc

brettmc commented Dec 31, 2021

Copy link
Copy Markdown
Contributor

Hi @Nevay - are you happy for this to be merged now?

@Nevay
Nevay force-pushed the feature/restructure branch from 9c209b7 to afb6618 Compare December 31, 2021 09:49
@Nevay
Nevay force-pushed the feature/restructure branch from afb6618 to bbc4fec Compare December 31, 2021 09:51
@Nevay

Nevay commented Dec 31, 2021

Copy link
Copy Markdown
Contributor Author

@brettmc yes, this should be finished now.
I kept Attributes::setAttribute() unchanged for now - method will be removed when we split Attributes into mutable builder and immutable attributes to ensure that readonly objects (e.g. SpanDataInterface) are not indirectly mutable.

@brettmc
brettmc merged commit 6ff577b into open-telemetry:main Jan 1, 2022
@Nevay Nevay mentioned this pull request Jan 6, 2022
Closed
@Nevay
Nevay deleted the feature/restructure branch April 19, 2023 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brettmc brettmc brettmc left review comments
@bobstrecansky bobstrecansky Awaiting requested review from bobstrecansky
@zsistla zsistla Awaiting requested review from zsistla
+1 more reviewer
@tidal tidal tidal approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Nevay @brettmc @tidal