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

RUM-3389 Add the Otel API feature integration tests #1995

Conversation

mariusc83
Copy link
Member

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 mariusc83 self-assigned this Apr 17, 2024
@mariusc83 mariusc83 force-pushed the mconstantin/rum-3389/add-trace-otel-feature-integration-tests branch from f45e21c to 76ae6a7 Compare April 17, 2024 12:54
@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2024

Codecov Report

Merging #1995 (76ae6a7) into feature/otel-support (37e2e35) will increase coverage by 0.12%.
Report is 2 commits behind head on feature/otel-support.
The diff coverage is 86.11%.

❗ Current head 76ae6a7 differs from pull request most recent head 989c4e0. Consider uploading reports for the commit 989c4e0 to get more accurate results

Additional details and impacted files
@@                   Coverage Diff                    @@
##           feature/otel-support    #1995      +/-   ##
========================================================
+ Coverage                 60.26%   60.38%   +0.12%     
========================================================
  Files                       808      808              
  Lines                     30184    30224      +40     
  Branches                   4945     4951       +6     
========================================================
+ Hits                      18188    18248      +60     
+ Misses                    10772    10760      -12     
+ Partials                   1224     1216       -8     
Files Coverage Δ
...dog/android/trace/internal/data/OtelTraceWriter.kt 88.89% <ø> (+3.70%) ⬆️
...e/src/main/java/com/datadog/trace/core/DDSpan.java 55.98% <0.00%> (-0.24%) ⬇️
...ternal/domain/event/OtelDdSpanToSpanEventMapper.kt 88.42% <88.57%> (+2.06%) ⬆️

... and 29 files with indirect coverage changes

@mariusc83 mariusc83 force-pushed the mconstantin/rum-3389/add-trace-otel-feature-integration-tests branch from 76ae6a7 to 8c37b96 Compare April 17, 2024 14:05
@mariusc83 mariusc83 marked this pull request as ready for review April 17, 2024 14:12
@mariusc83 mariusc83 requested review from a team as code owners April 17, 2024 14:12
@mariusc83 mariusc83 force-pushed the mconstantin/rum-3389/add-trace-otel-feature-integration-tests branch from 8c37b96 to 6a1b938 Compare April 17, 2024 14:16
@mariusc83 mariusc83 force-pushed the mconstantin/rum-4116/add-spanlink-support-for-otel-tracer branch 4 times, most recently from 40ec44a to f28a8cf Compare April 18, 2024 12:26
Base automatically changed from mconstantin/rum-4116/add-spanlink-support-for-otel-tracer to feature/otel-support April 18, 2024 13:18
@mariusc83 mariusc83 force-pushed the mconstantin/rum-3389/add-trace-otel-feature-integration-tests branch from 6a1b938 to 989c4e0 Compare April 18, 2024 13:37
xgouchet
xgouchet previously approved these changes Apr 18, 2024
0xnm
0xnm previously approved these changes Apr 18, 2024
import com.datadog.trace.bootstrap.instrumentation.api.AgentSpan
import io.opentelemetry.api.trace.Span

internal fun Span.lessSignificantTraceIdAsHexString(): String {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
internal fun Span.lessSignificantTraceIdAsHexString(): String {
internal fun Span.leastSignificantTraceIdAsHexString(): String {

Comment on lines 17 to 21
internal class BlockingWriterWrapper(wrapped: Writer) : Writer {
private val latches: LinkedList<CountDownLatch> = LinkedList()
private val currentTraceCount: AtomicInteger = AtomicInteger(0)

private val wrappedWriter = wrapped
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
internal class BlockingWriterWrapper(wrapped: Writer) : Writer {
private val latches: LinkedList<CountDownLatch> = LinkedList()
private val currentTraceCount: AtomicInteger = AtomicInteger(0)
private val wrappedWriter = wrapped
internal class BlockingWriterWrapper(private val wrappedWriter: Writer) : Writer {
private val latches: LinkedList<CountDownLatch> = LinkedList()
private val currentTraceCount: AtomicInteger = AtomicInteger(0)

Comment on lines +55 to +57
val latch = CountDownLatch(toWait)
synchronized(latches) {
latches.add(latch)
}
return latch.await(seconds.toLong(), TimeUnit.SECONDS)
Copy link
Member

Choose a reason for hiding this comment

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

In theory just a ConditionWatcher could be used here which polls the value of currentTraceCount, then in this case there is no need to create multiple latches. But fine as it is as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, I might change that if I have time but maybe in another ticket. I forgot about that ConditionWatcher

fun `set up`(forge: Forge) {
val datadogContextStorageClass =
Class.forName("com.datadog.android.trace.opentelemetry.DatadogContextStorage")
val constructor = datadogContextStorageClass.constructors.first { it.parameterCount == 1 }
Copy link
Member

Choose a reason for hiding this comment

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

constructor is a reserved keyword in Kotlin, even github highlights this word differently. To avoid confusion it is better to rename it to something like ctor or contructorRef.

@mariusc83 mariusc83 dismissed stale reviews from 0xnm and xgouchet via 0a423fe April 18, 2024 15:45
@mariusc83 mariusc83 force-pushed the mconstantin/rum-3389/add-trace-otel-feature-integration-tests branch from 989c4e0 to 0a423fe Compare April 18, 2024 15:45
@mariusc83 mariusc83 requested review from xgouchet and 0xnm April 18, 2024 15:45
@mariusc83 mariusc83 merged commit 3bd8514 into feature/otel-support Apr 19, 2024
25 checks passed
@mariusc83 mariusc83 deleted the mconstantin/rum-3389/add-trace-otel-feature-integration-tests branch April 19, 2024 07:40
@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.

4 participants