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

RUMM-2289 Add tracing sampling attribute #1092

Merged
merged 5 commits into from
Oct 19, 2022

Conversation

xgouchet
Copy link
Collaborator

What does this PR do?

Add the _dd.rule_psr attribute for RUM Resources to let customer know the sampling rate in APM

@xgouchet xgouchet requested a review from a team as a code owner October 18, 2022 10:54
@xgouchet xgouchet added the size-medium This PR is medium sized label Oct 18, 2022
@xgouchet xgouchet force-pushed the xgouchet/RUMM-2289/tracing_sampling_attribute branch from 50896a3 to 8b5a9c9 Compare October 18, 2022 14:58
Copy link
Contributor

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Looks good for the sampling logic 👍.

@codecov-commenter
Copy link

codecov-commenter commented Oct 19, 2022

Codecov Report

Merging #1092 (8b5a9c9) into develop (2b19144) will increase coverage by 0.03%.
The diff coverage is 90.00%.

@@             Coverage Diff             @@
##           develop    #1092      +/-   ##
===========================================
+ Coverage    83.23%   83.26%   +0.03%     
===========================================
  Files          273      273              
  Lines         9344     9348       +4     
  Branches      1502     1503       +1     
===========================================
+ Hits          7777     7783       +6     
  Misses        1148     1148              
+ Partials       419      417       -2     
Impacted Files Coverage Δ
...android/core/internal/sampling/RateBasedSampler.kt 87.50% <0.00%> (-12.50%) ⬇️
...in/kotlin/com/datadog/android/rum/RumAttributes.kt 0.00% <ø> (ø)
...n/kotlin/com/datadog/android/DatadogInterceptor.kt 87.21% <100.00%> (+0.15%) ⬆️
...roid/rum/internal/domain/scope/RumResourceScope.kt 97.93% <100.00%> (+0.02%) ⬆️
.../android/rum/internal/domain/scope/RumViewScope.kt 96.87% <100.00%> (ø)
...rc/main/java/com/datadog/opentracing/DDTracer.java 55.65% <0.00%> (-0.42%) ⬇️
...android/rum/internal/ndk/DatadogNdkCrashHandler.kt 88.65% <0.00%> (+0.54%) ⬆️
...m/datadog/android/core/internal/utils/ViewUtils.kt 81.82% <0.00%> (+9.09%) ⬆️
...ndroid/core/internal/persistence/file/EventMeta.kt 90.00% <0.00%> (+10.00%) ⬆️

"pattern": "^[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}$"
},
{
"type": "array",
Copy link
Collaborator

Choose a reason for hiding this comment

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

are we using this event ? I don't remember to have something like this yet in the SDK.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This schema is shared for all action children (i.e. Resources, Errors and Long Tasks). we used to only have an action.id: String, but now it can be either a String, or an array of Strings (for Frustration Signals)

@@ -362,7 +362,7 @@ internal open class RumViewScope(
type = errorType,
sourceType = event.sourceType.toSchemaSourceType()
),
action = context.actionId?.let { ErrorEvent.Action(it) },
action = context.actionId?.let { ErrorEvent.Action(listOf(it)) },
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we doing this ? Is there any situation where a LongTask will be caused by multiple actions ? Same question for all the other scopes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This can happen with frustration signals. The schema makes it so that an action.id can be either a string, or a list of string. Because we can't have such a thing in Kotlin, we go for the list of string which allows us to report both a single or multiple ids

Copy link
Contributor

@0xnm 0xnm left a comment

Choose a reason for hiding this comment

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

lgtm! I added one question regarding unexpected symbol.

@@ -167,7 +167,7 @@ class ClassDeserializerGenerator(
) {
val opt = if (nullable) "?" else ""
beginControlFlow(
"$assignee = $getter$opt.asJsonArray$opt.let { %L ->",
"$assignee = $getter$opt.asJsonArray$opt.let·{·%L·->",
Copy link
Contributor

Choose a reason for hiding this comment

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

why there is a change with middle dot instead of space here and in ClassGenerator?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is part of KotlinPoet internals to mark non-breaking space. This ensures that the KotlinPoet file writer won't break a line on any of those space.
This is because some classNames get very large and the line breaking happens at an arbitrary size without knowledge of the syntax, leading to code that don't compile

@xgouchet xgouchet merged commit 1c9981a into develop Oct 19, 2022
@xgouchet xgouchet deleted the xgouchet/RUMM-2289/tracing_sampling_attribute branch October 19, 2022 11:44
@xgouchet xgouchet added this to the 1.15.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size-medium This PR is medium sized
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants