RUM-3294 Bundle dd-trace-core code into the dd-sdk-android-trace module by mariusc83 · Pull Request #1907 · DataDog/dd-sdk-android

mariusc83 · 2024-03-11T12:24:50Z

What does this PR do?

A brief description of the change being made with this pull request.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

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)

0xnm · 2024-03-11T12:38:51Z

 org.jetbrains.kotlin:kotlin-stdlib-common:1.8.10                :  212 Kb
+org.jetbrains.kotlin:kotlin-stdlib-jdk7:1.8.10                  :  963 b 
+org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.8.10                  :  969 b 
 org.jetbrains.kotlin:kotlin-stdlib:1.8.10                       : 1598 Kb


in theory we should have only stdlib artifact https://kotlinlang.org/docs/whatsnew18.html#updated-jvm-compilation-target. Why do we have -jdk7 and -jdk8 here?

Ah :

com.squareup.okhttp3:okhttp:4.11.0 | +--- com.squareup.okio:okio:3.2.0 (*) | +--- org.jetbrains.kotlin:kotlin-stdlib:1.6.20 -> 1.8.10 (*) | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.20 -> 1.8.10

From okhttp

ok, they come because of the version bump done by Gradle. In theory we can remove those from dependencies explicitly, because all classes from -jdk7 and -jdk8 are in the main stdlib. But here they (these JDK7 and JDK8) they are probably empty by looking on the size, so no risk of getting DuplicateClassException.

yeah...I think I am going to try to do more cleaning at the end. At least moshi I want to remove it from there. It is going to be a big mess that's the reason why I want to do it at the end.

0xnm · 2024-03-11T13:02:20Z

+traceCore = [
+    "moshi",
+    "jctools",
+    "kotlin",


probably this one can be removed from the bundle

which one ? kotlin ? I think yes you're right.

codecov-commenter · 2024-03-12T08:20:07Z

Codecov Report

❌ Patch coverage is 24.85030% with 3514 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.84%. Comparing base (f18d527) to head (e952036).

⚠️ Current head e952036 differs from pull request most recent head e4674c8

Please upload reports for the commit e4674c8 to get more accurate results.

Files with missing lines	Patch %	Lines
...main/java/com/datadog/legacy/trace/api/Config.java	65.92%	173 Missing and 26 partials ⚠️
...om/datadog/trace/api/normalize/AntPathMatcher.java	0.00%	163 Missing ⚠️
...adog/trace/api/gateway/InstrumentationGateway.java	7.43%	161 Missing and 1 partial ⚠️
...ace/bootstrap/config/provider/ConfigConverter.java	26.74%	121 Missing and 16 partials ⚠️
...ava/com/datadog/trace/api/http/StoredByteBody.java	0.00%	125 Missing ⚠️
.../datadog/trace/api/iast/InstrumentationBridge.java	0.00%	122 Missing ⚠️
...ava/com/datadog/trace/api/http/StoredCharBody.java	0.00%	87 Missing ⚠️
...om/datadog/trace/api/MethodFilterConfigParser.java	3.66%	78 Missing and 1 partial ⚠️
.../trace/api/iast/telemetry/IastMetricCollector.java	0.00%	79 Missing ⚠️
...race/bootstrap/config/provider/ConfigProvider.java	65.33%	43 Missing and 26 partials ⚠️
... and 148 more

Additional details and impacted files

@@                    Coverage Diff                    @@
##           feature/otel-support    #1907       +/-   ##
=========================================================
- Coverage                 82.37%   55.84%   -26.54%     
=========================================================
  Files                       490      789      +299     
  Lines                     17411    29274    +11863     
  Branches                   2610     4831     +2221     
=========================================================
+ Hits                      14342    16346     +2004     
- Misses                     2348    11872     +9524     
- Partials                    721     1056      +335

Files with missing lines	Coverage Δ
.../legacy/trace/common/sampling/AbstractSampler.java	`0.00% <ø> (ø)`
...cy/trace/common/sampling/RateByServiceSampler.java	`60.87% <100.00%> (ø)`
...va/com/datadog/legacy/trace/common/util/Clock.java	`75.00% <ø> (ø)`
...va/com/datadog/opentelemetry/trace/OtelTracer.java	`100.00% <100.00%> (ø)`
.../src/main/java/com/datadog/opentracing/DDSpan.java	`51.65% <ø> (ø)`
...in/java/com/datadog/opentracing/DDSpanContext.java	`64.62% <ø> (ø)`
...rc/main/java/com/datadog/opentracing/DDTracer.java	`57.02% <ø> (ø)`
...ava/com/datadog/opentracing/DefaultLogHandler.java	`10.00% <ø> (ø)`
...ain/java/com/datadog/opentracing/PendingTrace.java	`70.73% <ø> (ø)`
...atadog/opentracing/decorators/DBTypeDecorator.java	`20.00% <ø> (ø)`
... and 185 more

... and 220 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The base branch was changed.

0xnm · 2024-03-12T08:52:35Z

@@ -301,6 +301,14 @@ ktor = [
    "ktorGson"
 ]

+traceCore = [


should we rename this one since there is no more trace-core module?

actually these deps are still related with the trace core code ported from JAVA so it is better to keep them like that. In a later task I might clean these deps a bit more, I really want to get rid of moshi at least there.

xgouchet · 2024-03-12T09:02:48Z

 import,androidx.versionedparcelable,Apache-2.0,Copyright 2018 The Android Open Source Project
 import,androidx.viewpager,Apache-2.0,Copyright 2018 The Android Open Source Project
 import,androidx.work,Apache-2.0,Copyright 2018 The Android Open Source Project
+import,com.android.tools,GNU General Public License, Version 3, 29 June 2007. Copyright (C) 2007 Free Software Foundation


The copyright notice here feels weird. I would expect the copyright to mention Google or Android, not the FSF

there's no placeholder in their copyright so I don't really know exactly the format. Do you have another example somewhere ?

nevermind I think I've found one

0xnm · 2024-03-12T10:34:19Z

 import,androidx.versionedparcelable,Apache-2.0,Copyright 2018 The Android Open Source Project
 import,androidx.viewpager,Apache-2.0,Copyright 2018 The Android Open Source Project
 import,androidx.work,Apache-2.0,Copyright 2018 The Android Open Source Project
+import,com.android.tools,GNU General Public License, Version 3, 29 June 2007. Copyright 2024 Datadog, Inc.


probably everything under com.android.tools is under Apache-2.0 license by clicking different items under https://mvnrepository.com/artifact/com.android.tools. So the same license as above (and same Copyright)

Yeah some mention GNU some Apache. I will add Apache instead.

0xnm · 2024-03-12T10:35:04Z

 import,com.github.bumptech.glide,"Simplified BSD License/The Apache Software License, Version 2.0","Copyright 2014 Google, Inc. All rights reserved, Copyright (c) 2013. Bump Technologies Inc. All Rights Reserved."
 import,com.facebook.fresco,MIT,"Copyright (c) Facebook, Inc. and its affiliates"
-import,com.github.spotbugs,"GNU LESSER GENERAL PUBLIC LICENSE, Version 2.1","Copyright (C) 1991, 1999 Free Software Foundation, Inc."
+import,com.github.spotbugs,GNU LESSER GENERAL PUBLIC LICENSE, Version 2.1. Copyright 2024 Datadog, Inc.


this should be reverted according to https://github.com/spotbugs/spotbugs/blob/master/LICENSE

yeah I really do not know anymore honestly what is correct there. I will put this back.

The merge-base changed after approval.

mariusc83 self-assigned this Mar 11, 2024

mariusc83 requested review from a team as code owners March 11, 2024 12:24

0xnm reviewed Mar 11, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-3294/bundle-dd-trace-core-into-dd-sdk-trace branch from 791c000 to 7f8f319 Compare March 11, 2024 13:01

0xnm previously approved these changes Mar 11, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-3294/bundle-dd-trace-core-into-dd-sdk-trace branch 2 times, most recently from 67aa40b to fdb7356 Compare March 11, 2024 16:38

mariusc83 changed the title ~~RUM-3294 Bundle dd-trace-core into dd-sdk-android-trace aar artifact~~ RUM-3294 Bundle dd-trace-core code into the dd-sdk-android-trace module Mar 11, 2024

Base automatically changed from mconstantin/rum-3415/add-opentelemety-use-cases-in-sample-app to feature/otel-support March 12, 2024 08:48

mariusc83 requested a review from 0xnm March 12, 2024 08:49

mariusc83 force-pushed the mconstantin/rum-3294/bundle-dd-trace-core-into-dd-sdk-trace branch from fdb7356 to 584607e Compare March 12, 2024 08:54

0xnm previously approved these changes Mar 12, 2024

View reviewed changes

xgouchet suggested changes Mar 12, 2024

View reviewed changes

mariusc83 dismissed 0xnm’s stale review via df46ba5 March 12, 2024 09:26

mariusc83 force-pushed the mconstantin/rum-3294/bundle-dd-trace-core-into-dd-sdk-trace branch from 584607e to df46ba5 Compare March 12, 2024 09:26

mariusc83 requested review from 0xnm and xgouchet March 12, 2024 10:14

0xnm requested changes Mar 12, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-3294/bundle-dd-trace-core-into-dd-sdk-trace branch from df46ba5 to e952036 Compare March 12, 2024 10:45

0xnm previously approved these changes Mar 12, 2024

View reviewed changes

mariusc83 force-pushed the feature/otel-support branch from 2bd30a2 to f18d527 Compare March 12, 2024 16:18

RUM-3294 Bundle dd-trace-core into dd-sdk-android-trace

e4674c8

mariusc83 force-pushed the mconstantin/rum-3294/bundle-dd-trace-core-into-dd-sdk-trace branch from e952036 to e4674c8 Compare March 12, 2024 16:21

0xnm approved these changes Mar 12, 2024

View reviewed changes

xgouchet approved these changes Mar 13, 2024

View reviewed changes

mariusc83 merged commit 5506818 into feature/otel-support Mar 13, 2024

mariusc83 deleted the mconstantin/rum-3294/bundle-dd-trace-core-into-dd-sdk-trace branch March 13, 2024 12:55

xgouchet added this to the 2.11.x milestone Jul 31, 2024

Uh oh!

Conversation

mariusc83 commented Mar 11, 2024

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

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!

codecov-commenter commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Mar 12, 2024 •

edited

Loading