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

Rename ParentOrElse to ParentBased and generalize to support all cases #1577

Merged
merged 12 commits into from
Aug 28, 2020

Conversation

dengliming
Copy link
Member

@dengliming dengliming commented Aug 22, 2020

closes #1572

@codecov
Copy link

codecov bot commented Aug 22, 2020

Codecov Report

Merging #1577 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1577      +/-   ##
============================================
+ Coverage     86.85%   86.88%   +0.03%     
- Complexity     1392     1393       +1     
============================================
  Files           162      162              
  Lines          5369     5397      +28     
  Branches        513      519       +6     
============================================
+ Hits           4663     4689      +26     
- Misses          523      524       +1     
- Partials        183      184       +1     
Impacted Files Coverage Δ Complexity Δ
...main/java/io/opentelemetry/sdk/trace/Samplers.java 93.61% <100.00%> (+2.70%) 14.00 <2.00> (+1.00)
...io/opentelemetry/sdk/trace/config/TraceConfig.java 95.00% <100.00%> (ø) 6.00 <1.00> (ø)
...y/sdk/metrics/aggregator/DoubleMinMaxSumCount.java 96.07% <0.00%> (-3.93%) 6.00% <0.00%> (ø%)

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 ca85d8f...4d0ba9c. Read the comment docs.

@Oberon00
Copy link
Member

@jkwatson
Copy link
Contributor

Just renaming is insufficient. https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#parentbased

yes, I don't think this will close #1572, but I think it's a step along the way. If we don't close #1572 with this PR, are you ok with it as a first step, @Oberon00 ?

@Oberon00
Copy link
Member

Oberon00 commented Aug 22, 2020

Yeah, sorry I just meant "insufficient to close #1572". 😃 EDIT: So I think the "closes #1572" must be removed from the PR description before merging this.

@jkwatson
Copy link
Contributor

@dengliming can you either 1) remove the closes from the PR description or 2) address making sure the implementation fully addresses the spec?

Thanks!

@dengliming
Copy link
Member Author

dengliming commented Aug 24, 2020

@jkwatson Sorry. just forgot it. : ) . I will do the implementation fully addresses the spec recently. Can you assign to me?

@dengliming dengliming marked this pull request as draft August 24, 2020 16:02
@jkwatson
Copy link
Contributor

@jkwatson Sorry. just forgot it. : ) . I will do the implementation fully addresses the spec recently. Can you assign to me?

Done, and thanks for taking that on!

@dengliming dengliming marked this pull request as ready for review August 26, 2020 16:06
@dengliming dengliming changed the title Rename ParentOrElse to ParentBased Rename ParentOrElse to ParentBased and generalize to support all cases Aug 26, 2020
@jkwatson
Copy link
Contributor

This is looking pretty good to me. Thanks for putting in the work!

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Mostly looks good, thanks

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Sorry noticed one more small comment on the javadoc

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Thanks!

@jkwatson jkwatson merged commit f680094 into open-telemetry:master Aug 28, 2020
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.

Rename ParentOrElse to ParentBased and generalize to support all cases
5 participants