fix:Add factories for SamplingResult #3354 by Rocksnake · Pull Request #3360 · open-telemetry/opentelemetry-java

Rocksnake · 2021-07-01T09:27:40Z

Add three methods to get the factory to replace the original create
Modify all the places where SamplingResult.create is used in the project
Write unit test and pass the test

Remarks：since the create method with parameters is used in RateLimitingSampler, I keep the versions with Attributes

linux-foundation-easycla · 2021-07-01T09:27:43Z

The committers are authorized under a signed CLA.

✅ BrianYan (384da9d)

Rocksnake · 2021-07-01T12:56:18Z

Could anybody help to approve running these workflows, great thanks!
This PR resolves #3354

Oberon00 · 2021-07-01T13:35:01Z

For the future: Please also mention the ticket number in the description as "Fixes #3354". GitHub does not link issues from the title.

Could anybody help to approve running these workflows, great thanks!

I have an "Approve and run" button and clicked it and it seemed to have started but now it seems stuck. Probably an actual maintainer needs to approve the run. EDIT: Oh, it seems to have worked. Probably each new push has to be re-approved.

jkwatson · 2021-07-01T19:37:44Z

+   * @return A {@link SamplingResult} with the attributes equivalent to {@code attributes} and the
+   * {@link SamplingDecision#RECORD_AND_SAMPLE}
+   */
+  static SamplingResult recordAndSample(Attributes attributes) {


I wouldn't create these overloads with attributes. As @anuraaga mentioned, the existing method works fine for that, and actually does create a new object when being called.

Thanks, but in the project, RateLimitingSampler calls the method with parameters. I think the method with parameters is necessary when we need specific sampling. I'm a first-time contributor, I hope you can give me some guidance.

@Rocksnake Thanks for the help - as per the below comment, we won't be (can't) removing the create methods. We should continue to use create for creating the results with attributes

Ok, thanks. And in my last commit, I have restored the original create method.

jkwatson · 2021-07-01T19:38:58Z

  static final SamplingResult EMPTY_RECORDED_SAMPLING_RESULT =
      ImmutableSamplingResult.createWithoutAttributes(SamplingDecision.RECORD_ONLY);

+  static SamplingResult getEmptyRecordedAndSampledSamplingResult() {


small thing, but I might just call this recordAndSample(). It's much easier to read and there's fewer words to parse through to get to what it's doing. I know this isn't public, but I think it would be more readable to have a shorter, less wordy method name. Same for the other methods.

actually...do we even need these methods at all? The things that need them can just use the fields like they are today. I think we should stick with that.

yes, let's get rid of these methods and just let the package access the package-protected constants like it has been.

@Rocksnake please get rid of these methods. thanks!

@Rocksnake I think you missed this comment

jkwatson · 2021-07-01T19:42:12Z

-   * @param decision The decision made on the span.
   * @return A {@link SamplingResult} with empty attributes and the provided {@code decision}.
   */
-  static SamplingResult create(SamplingDecision decision) {


we can't delete this public method. That would be a breaking change.

is it too early to deprecate?

I think it's fine to keep it here. we could deprecate at some point, but it's not harmful or bad to use this method, just slightly confusing since it won't actually create anything.

jkwatson · 2021-07-01T19:43:31Z

@Rocksnake please run the ./gradlew build task and make sure to add any changes in the docs/apidiffs directory to this PR, since it is a change to the public APIs in the SDK. Thanks!

jkwatson · 2021-07-02T03:28:50Z

+***! MODIFIED INTERFACE: PUBLIC ABSTRACT io.opentelemetry.sdk.trace.samplers.SamplingResult  (not serializable)
+	===  CLASS FILE FORMAT VERSION: 52.0 <- 52.0
+	+++! NEW METHOD: PUBLIC(+) STATIC(+) io.opentelemetry.sdk.trace.samplers.SamplingResult drop()
+	+++! NEW METHOD: PUBLIC(+) STATIC(+) io.opentelemetry.sdk.trace.samplers.SamplingResult drop(io.opentelemetry.api.common.Attributes)


we'd still like to get rid of these overloads that have attributes as a parameter. Thanks.

I think this kind of call case is also necessary.

But I will try my best to think about how to replace these calls with attributes as parameter.

Based on all the above, I feel that we should keep the old calls with attributes, and use the new short method in the future development process. I have achieved this idea and hope you approve.

Rocksnake · 2021-07-03T03:30:20Z

I have resolve all the problems, could anyone help to approve running these workflows and merge in main branch, great thanks!

trask · 2021-07-03T18:51:25Z

   * @param decision The decision made on the span.
   * @return A {@link SamplingResult} with empty attributes and the provided {@code decision}.
   */
+  @SuppressWarnings("UngroupedOverloads")


I don't think you need this suppress warnings

trask · 2021-07-03T18:52:31Z

-   * @param decision The decision made on the span.
   * @return A {@link SamplingResult} with empty attributes and the provided {@code decision}.
   */
-  static SamplingResult create(SamplingDecision decision) {


is it too early to deprecate?

jkwatson · 2021-07-03T19:20:45Z

   * @return A {@link SamplingResult} with the attributes equivalent to {@code attributes} and the
   *     provided {@code decision}.
   */
+  @SuppressWarnings("UngroupedOverloads")


It would be better if we can make sure we group the overloads so we don't need this warning.

I have removed them, could you please trigger the check for me again. Thanks~

anuraaga

Thanks! Just @jkwatson's coment for removing the unnecessary private methods and a small suggestion to the javadocs

anuraaga · 2021-07-05T07:24:32Z

+   * <p>This is meant for use by custom {@link Sampler} implementations.
+   *
+   * @return A {@link SamplingResult} with empty attributes and the provided {@code decision}.


Suggested change

* <p>This is meant for use by custom {@link Sampler} implementations.

*

* @return A {@link SamplingResult} with empty attributes and the provided {@code decision}.

* <p>This is meant for use by custom {@link Sampler} implementations and is equivalent to calling

* {@code SamplingResult.create(SamplingDecision.RECORD_AND_SAMPLE)}.

*

* @return A {@link SamplingResult} with empty attributes and the provided {@code decision}.

Let's add something like this to the three methods.

I have added them, could you check for me and merge it please. Thanks.

Rocksnake · 2021-07-06T02:53:26Z

Does anyone know the reason for this problem, I need you help to solve it. The last check is no-error, I only modify three lines of annotation this time.

Rocksnake · 2021-07-07T13:27:53Z

Could anyone merge this branch into main, I have solved all above problem, great thanks.

jkwatson · 2021-07-07T15:38:15Z

Could anyone merge this branch into main, I have solved all above problem, great thanks.

You still haven't removed the methods I asked you to. Please do so.

Rocksnake · 2021-07-14T11:23:46Z

Could anyone merge this branch into main, I have solved all above problem, great thanks.

You still haven't removed the methods I asked you to. Please do so.

I haved removed these methods, please check again, thank you!

jkwatson · 2021-07-14T14:50:23Z

This PR needs to be rebased against the main branch before we can merge it.

jkwatson · 2021-07-19T16:11:23Z

@Rocksnake this needs a rebase. Will you be able to get to it soon?

Rocksnake · 2021-07-20T00:27:25Z

@Rocksnake this needs a rebase. Will you be able to get to it soon?

Ok, I will rebased it this day.

fix:Add factories for SamplingResult

384da9d

Rocksnake requested review from Oberon00, anuraaga, arminru, bogdandrutu, carlosalberto, jkwatson, pavolloffay, thisthat and tylerbenson as code owners July 1, 2021 09:27

fix:modify package import

72bfccf

Oberon00 linked an issue Jul 1, 2021 that may be closed by this pull request

Add factories for SamplingResult #3354

Closed

modify the problem of excessive line length

2d3c9bb

jkwatson reviewed Jul 1, 2021

View reviewed changes

Comment thread sdk/trace/src/main/java/io/opentelemetry/sdk/trace/samplers/SamplingResult.java

keep both old and new version of create method

9bef73f

Rocksnake requested a review from a user July 2, 2021 02:53

jkwatson reviewed Jul 2, 2021

View reviewed changes

Rocksnake requested a review from jkwatson July 2, 2021 05:02

fix:remove the method with attributes

65d19b2

trask reviewed Jul 3, 2021

View reviewed changes

jkwatson reviewed Jul 3, 2021

View reviewed changes

remove UngroupedOverloads suppress warning

fe4c3c7

Rocksnake requested a review from jkwatson July 4, 2021 01:58

anuraaga reviewed Jul 5, 2021

View reviewed changes

feat:add javadocs

d45e523

Rocksnake requested a review from anuraaga July 5, 2021 17:13

delete some methods and handle the conflicts

2027653

Rocksnake closed this Jul 20, 2021

Rocksnake deleted the fix/add_factories_for_samplingResult branch July 20, 2021 01:49

Rocksnake mentioned this pull request Jul 20, 2021

Add factories for SamplingResult #3410

Merged

Conversation

Rocksnake commented Jul 1, 2021

Uh oh!

linux-foundation-easycla Bot commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rocksnake commented Jul 1, 2021

Uh oh!

Oberon00 commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkwatson commented Jul 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rocksnake commented Jul 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anuraaga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rocksnake commented Jul 6, 2021

Uh oh!

Rocksnake commented Jul 7, 2021

Uh oh!

jkwatson commented Jul 7, 2021

Uh oh!

Rocksnake commented Jul 14, 2021

Uh oh!

jkwatson commented Jul 14, 2021

Uh oh!

jkwatson commented Jul 19, 2021

Uh oh!

Rocksnake commented Jul 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

linux-foundation-easycla Bot commented Jul 1, 2021 •

edited

Loading

Oberon00 commented Jul 1, 2021 •

edited

Loading