Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge Otel feature branch #2077

Merged
merged 88 commits into from
Jun 12, 2024
Merged

Conversation

mariusc83
Copy link
Collaborator

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)

mariusc83 and others added 30 commits March 12, 2024 17:16
…trace-core-into-dd-sdk-trace

RUM-3294 Bundle dd-trace-core code into the dd-sdk-android-trace module
…provide-sampling-tags-for-otel-spans

RUM-3516 Provide the correct sampling priority for our Span events based on APM new rules
…ore-tracer-unit-tests

RUM-3294 Add the Core Tracer tests
…re-tracer-logger-implementation

RUM-3405 Provide core tracer logger implementation
…ndle-with-rum-capability

RUM-3249 Provide the bundleWithRum capability for OtelTracer
…el-contextstorage-custom-implementation

RUM-3836 Provide the DatadogContextStorage for Opentelemetry
…el-bundle-with-logs-feature

RUM-3250 Provide Otel bundle with logs feature
@mariusc83 mariusc83 self-assigned this Jun 11, 2024
@mariusc83 mariusc83 requested review from a team as code owners June 11, 2024 07:51
@codecov-commenter
Copy link

codecov-commenter commented Jun 11, 2024

Codecov Report

Attention: Patch coverage is 38.32171% with 2477 lines in your changes missing coverage. Please review.

Project coverage is 67.59%. Comparing base (d3f69b6) to head (3d1a62e).
Report is 3 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff              @@
##           develop    #2077       +/-   ##
============================================
- Coverage    83.19%   67.59%   -15.60%     
============================================
  Files          495      735      +240     
  Lines        17773    27019     +9246     
  Branches      2685     4563     +1878     
============================================
+ Hits         14785    18261     +3476     
- Misses        2256     7540     +5284     
- Partials       732     1218      +486     
Files Coverage Δ
...va/com/datadog/opentelemetry/trace/OtelTracer.java 100.00% <100.00%> (ø)
.../trace/opentelemetry/internal/FeatureSdkCoreExt.kt 100.00% <100.00%> (ø)
.../android/trace/opentelemetry/internal/OtelScope.kt 100.00% <100.00%> (ø)
.../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% <ø> (ø)
...adog/legacy/trace/common/writer/LoggingWriter.java 0.00% <ø> (ø)
.../src/main/java/com/datadog/opentracing/DDSpan.java 50.56% <ø> (ø)
...in/java/com/datadog/opentracing/DDSpanContext.java 64.06% <ø> (ø)
...rc/main/java/com/datadog/opentracing/DDTracer.java 57.64% <ø> (ø)
... and 196 more

... and 94 files with indirect coverage changes

@jhgilbert jhgilbert self-requested a review June 11, 2024 17:01
See the dedicated [Datadog SDK Android for OpenTelemetry documentation][2] to learn how to add a parent span to network requests made by the `OkHttp` library.

[1]: https://docs.datadoghq.com/real_user_monitoring/android/advanced_configuration/#automatically-track-network-requests
[2]: https://docs.datadoghq.com/tracing/trace_collection/custom_instrumentation/android?tab=kotlin

Choose a reason for hiding this comment

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

Is the second link expected to work yet? It doesn't currently but I couldn't find an alternative, guessing these docs just aren't published yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes the idea is to have this merge and release in the next version and then merge the documentation PR which is already opened.

.gitlab-ci.yml Outdated
Comment on lines 110 to 113
# We need separated steps for tracing module tests as these cannot be run with the CI visibility agent enabled. Also they need to be executed with
# the daemon capability as the worker threads are running as a daemon.

test:trace-debug:
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still need dedicated job stages after all the changes done?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure but I will check after I merge this and add a PR to clean the gitlab-ci if it turns out that it can be done without.

Comment on lines 42 to 44
buildFeatures {
buildConfig = true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this one is not needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will double check it.

Comment on lines +6 to +7
com.squareup.okhttp3:okhttp:4.11.0 : 768 Kb
com.squareup.okio:okio-jvm:3.2.0 : 337 Kb
Copy link
Contributor

Choose a reason for hiding this comment

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

where it comes from? trace library shouldn't have its own transport, I was thinking we shouldn't have it after Moshi removal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am pretty sure it is coming from something that is being used and not able to remove it but will double check it.

@@ -413,85 +424,501 @@ public final class com/datadog/exec/DaemonThreadFactory : java/util/concurrent/T
public fun newThread (Ljava/lang/Runnable;)Ljava/lang/Thread;
}

public class com/datadog/opentracing/DDSpan : com/datadog/trace/api/interceptor/MutableSpan, io/opentracing/Span {
Copy link
Contributor

Choose a reason for hiding this comment

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

are we going to verify binary compatibility of Java classes exposed through our Kotlin API? Do we break anything?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these classes are exposed only for internal usage, we do not have a Java sample app for now but I did some try - outs. In any case these classes should not be used by our customers directly. It is the same case for the old Java code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, they were still publicly available (since Java doesn't have internal visibility modifier) and customer could downcast Span to DDSpan to do something, we didn't make any statement in the Javadoc saying they shouldn't be considered as a part of the public API. But I guess at the current point there is nothing we can do.

@mariusc83 mariusc83 requested a review from 0xnm June 12, 2024 08:57
@mariusc83 mariusc83 merged commit f2af2ad into develop Jun 12, 2024
20 checks passed
@mariusc83 mariusc83 deleted the mconstantin/merge-otel-into-develop branch June 12, 2024 09:20
@xgouchet xgouchet added this to the 2.11.x milestone Jul 31, 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.

5 participants