Skip to content

RUM-7527: Deprecate Datadog GlobalTracer class#2438

Merged
ambushwork merged 1 commit into
developfrom
yl/rename-global-tracer
Dec 9, 2024
Merged

RUM-7527: Deprecate Datadog GlobalTracer class#2438
ambushwork merged 1 commit into
developfrom
yl/rename-global-tracer

Conversation

@ambushwork

@ambushwork ambushwork commented Dec 6, 2024

Copy link
Copy Markdown
Member

What does this PR do?

Deprecate Datadog GlobalTracer class to avoid confusion with opentracing GlobalTracer

Motivation

RUM-7527

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@ambushwork
ambushwork requested review from a team as code owners December 6, 2024 09:34

@0xnm 0xnm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure we can remove it, because even though it is not used for the means of the Android SDK, some users may rely on this as an exported symbol for their usage with com.datadog.trace.api.Tracer (this would be strange though).

Maybe it is better to deprecate it instead?

@mariusc83

Copy link
Copy Markdown
Member

I'm not sure we can remove it, because even though it is not used for the means of the Android SDK, some users may rely on this as an exported symbol for their usage with com.datadog.trace.api.Tracer (this would be strange though).

Maybe it is better to deprecate it instead?

I really want to avoid the confusion here, I would say removing is better as there should not be any usage of the CoreTracer directly. This is our internal tracer and it is the only one that implements the com.datadog.trace.api.Tracer. If someone uses the CoreTracer directly then it will be a mistake.

@0xnm

0xnm commented Dec 6, 2024

Copy link
Copy Markdown
Member

Yes, for sure, its usage is a misuse, but at the same time it is binary-incompatible change and we can do it only for the next major release for the public-facing API (we cannot say that there is no anyone using this symbol for some strange needs).

At the same time java.lang.Deprecated doesn't allow to supply the message, but kotlin.Deprecated allows to do it (maybe we can use it in the Java code?).

But anyway, it is your call.

@codecov-commenter

codecov-commenter commented Dec 6, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.10%. Comparing base (661033b) to head (e18c280).
Report is 2 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2438      +/-   ##
===========================================
+ Coverage    70.06%   70.10%   +0.04%     
===========================================
  Files          769      769              
  Lines        28511    28509       -2     
  Branches      4778     4778              
===========================================
+ Hits         19976    19986      +10     
+ Misses        7190     7185       -5     
+ Partials      1345     1338       -7     
Files with missing lines Coverage Δ
.../main/java/com/datadog/trace/api/GlobalTracer.java 0.00% <ø> (ø)
...ava/com/datadog/trace/api/profiling/Profiling.java 0.00% <ø> (ø)
...n/com/datadog/android/okhttp/DatadogInterceptor.kt 60.00% <ø> (-0.67%) ⬇️

... and 27 files with indirect coverage changes

@xgouchet

xgouchet commented Dec 6, 2024

Copy link
Copy Markdown
Contributor

I would tend to agree with @0xnm here: removing it might break using for some of our customers.
Adding a Deprecated annotation (and adding javadoc to explain why) seems like a better first step.
Awe could additionally add some telemetry to track the usage, and if we are sure no one uses it, we can then delete it.

@ambushwork
ambushwork force-pushed the yl/rename-global-tracer branch from e7742db to 98c6002 Compare December 6, 2024 16:36
@ambushwork
ambushwork requested review from 0xnm and mariusc83 December 6, 2024 16:36

@0xnm 0xnm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Lgtm. I added one suggestion and probably PR title should be updated (we are not removing anything yet).

@ambushwork
ambushwork force-pushed the yl/rename-global-tracer branch from 98c6002 to e18c280 Compare December 9, 2024 08:47
@ambushwork
ambushwork merged commit d737ab4 into develop Dec 9, 2024
@ambushwork
ambushwork deleted the yl/rename-global-tracer branch December 9, 2024 09:38
@ambushwork ambushwork changed the title RUM-7527: Remove unused Datadog GlobalTracer class RUM-7527: Deprecate Datadog GlobalTracer class Dec 9, 2024
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.

6 participants