Prevent public API related spans from being recycled

[eyalkoren]

What does this PR do?

In order to work around misuse of the public activation API, related spans will not be recycled.
This way we can ensure there is no improper usage of recycled-and-still-referenced span objects at the cost of creating some garbage.

Checklist

• This is an enhancement of existing features, or a new feature in existing plugins
  • I have updated CHANGELOG.asciidoc
  • I have added tests that prove my fix is effective or that my feature works
  • I have made corresponding changes to the documentation

I decided not to add anything to the documentation about that, as I think it would only cause confusion.

[ghost]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Build Cause: Pull request #1859 updated

• Start Time: 2021-06-17T11:32:59.431+0000

• Duration: 51 min 27 sec

• Commit: 3463919

Test stats 🧪

Test	Results
Failed	0
Passed	2246
Skipped	18
Total	2264

Trends 🧪

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	2246
Skipped	18
Total	2264

[SylvainJuge]

LGTM, I'm fine with having webflux test app rely on internal API for now, we can revisit this later if required to. 99.99% of the time this app will only be used for unit testing.

[felixbarny]

Have you considered instrumenting AbstractSpanImpl directly and incrementing the reference there? The advantage is that would also work with older versions of the API.

[eyalkoren]

If you mean the constructor, then - yes, I considered but preferred this.
As far as I see it - there is no problem to any current API user (besides the one that reported this issue) and all future users will use the newer API version.

[felixbarny]

👌

[CoderLan0668]

Using the public api to develop an external plugin, but an error was thrown in unit test:

the problem was caused by the invoking of the public api co.elastic.apm.api.ElasticApm#currentSpan , which will incrementReferences .

In order to work around misuse of the public activation API, related spans will not be recycled.

i think this PR is terrible, as it makes the RecyclingValidation become invalid.

So what should i do now, just disable the RecyclingValidation like this PR do?

[eyalkoren]

Thanks for the honest feedback.

We've seen enough misuse that caused a lot of grief to our community, which made us decide that the performance gains of recycling do not worth the pain, only in the narrow context of public API spans/transactions.
The problem with recycled "dirty" spans is that they may be reused while still containing irrelevant data, which can be very difficult to debug, or even track.

This PR only makes sure that spans/transactions "touched" by the public API pass away in the natural Java way, which is garbage collection, instead of being recycled. So if your recycling validation would have passed, that would be a bug in this PR that requires fixing 🙂 .

So what should i do now, just disable the RecyclingValidation like this PR do?

Yes, for the time being.
We intend to put more efforts soon to expose all our testing capabilities better for external plugins developers, so if I am to get something constructive from your feedback, that would be to add an abstract test class for external plugins that disables this check. Would that be a good solution in your opinion?