Remove deprecated APIs #414
Conversation
ghost
assigned 
pavolloffay
requested review from
yurishkuro,
vprithvi and
jpkrohling
and removed request for
yurishkuro and
vprithvi
Codecov Report
@@ Coverage Diff @@
## master #414 +/- ##
============================================
+ Coverage 82.8% 87.41% +4.61%
Complexity 512 512
============================================
Files 69 66 -3
Lines 2105 1955 -150
Branches 263 258 -5
============================================
- Hits 1743 1709 -34
+ Misses 273 156 -117
- Partials 89 90 +1
Continue to review full report at Codecov.
|
Internal use of deprecated API has been removed in previous PRs - just to keep this clean |
pavolloffay
force-pushed
the
remove-deprecated-api-2
branch
2 times, most recently
from
4d8fb5a to
a9d5560
Compare
Ok, I personally would have preferred to keep this here, so that the actual effects of this PR could be easily measured. |
|
@yurishkuro could you please have a look? |
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
pavolloffay
force-pushed
the
remove-deprecated-api-2
branch
from
60a3e8f to
6ad09bd
Compare
| public RemoteReporter(Sender sender, int flushInterval, int maxQueueSize, Metrics metrics) { | ||
| this(sender, flushInterval, maxQueueSize, DEFAULT_CLOSE_ENQUEUE_TIMEOUT_MILLIS, metrics); | ||
| } | ||
|
|
||
| RemoteReporter(Sender sender, int flushInterval, int maxQueueSize, int closeEnqueueTimeout, |
There was a problem hiding this comment.
should we remove this ctor as well? Having public ctors is what leads to proliferation of deprecated methods in the first place, it's better to use builder pattern for most core components. Builder can be overkill for smaller classes, but RemoteReporter is a critical component that can continue evolving.
E.g. I see that we already use builder for RemoteSampler.
There was a problem hiding this comment.
It is package private, so not exposed to the world.
I can make it private in this PR.
| @@ -43,47 +43,12 @@ | |||
| * http://localhost:14268/api/traces | |||
| * | |||
| * Uses the default {@link okhttp3.OkHttpClient} which uses {@link okhttp3.ConnectionPool#ConnectionPool()}. | |||
| * Use {@link HttpSender#HttpSender(java.lang.String, int, okhttp3.OkHttpClient)} to adjust parameters. | |||
| * Use {@link HttpSender.Builder} if you need to add more parameters | |||
| */ | |||
| public HttpSender(String endpoint) { | |||
There was a problem hiding this comment.
any reason to keep this ctor?
There was a problem hiding this comment.
Not really, It would be better to remove it to make the API more consistent. We can do 0.28.0 with more deprecations ASAP, then remove it
|
@yurishkuro I will merge this and address other comments in a separate PR as it's not related to removing deprecated APIs. |
Resolves #395 #413
I didn't removeI did in the second commitTracer.Builder(name, reporter, samper)#413Related PRs which removed usage of deprecated APIs from internal code
Signed-off-by: Pavol Loffay ploffay@redhat.com