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.

Sign up for GitHub

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

Jump to bottom

RUM-886 Provide session replay data in configuration telemetry #1611

Merged
merged 1 commit into from
Sep 12, 2023
Merged

RUM-886 Provide session replay data in configuration telemetry #1611

merged 1 commit into from
Sep 12, 2023

Conversation

mariusc83
Copy link
Member

@mariusc83 mariusc83 commented Sep 12, 2023 •
edited
Loading

What does this PR do?

In this PR we are adding the information for the Session Replay feature configuration in our related Telemetry event.

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 force-pushed the mconstantin/rum-886/report-sr-in-configuration-telemetry branch from 4584101 to e22c4df Compare September 12, 2023 11:05
@mariusc83 mariusc83 self-assigned this Sep 12, 2023
xgouchet
xgouchet reviewed Sep 12, 2023
View reviewed changes
Copy link
Member

@xgouchet xgouchet left a comment

Choose a reason for hiding this comment

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

LGTM

@mariusc83 mariusc83 force-pushed the mconstantin/rum-886/report-sr-in-configuration-telemetry branch 2 times, most recently from 6751fe7 to 8cc72eb Compare September 12, 2023 12:59
@mariusc83 mariusc83 marked this pull request as ready for review September 12, 2023 12:59
@mariusc83 mariusc83 requested a review from a team as a code owner September 12, 2023 12:59
0xnm
0xnm reviewed Sep 12, 2023
View reviewed changes
...-android-rum/src/main/kotlin/com/datadog/android/telemetry/internal/TelemetryEventHandler.kt Outdated
val sessionReplayFeatureContext =
sdkCore.getFeatureContext(Feature.SESSION_REPLAY_FEATURE_NAME)
val sessionReplaySampleRate = sessionReplayFeatureContext[SESSION_REPLAY_SAMPLE_RATE_KEY]
?.let { it as? Long }
Copy link
Member

Choose a reason for hiding this comment

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

why do we need let? we should be able to write as sessionReplayFeatureContext[SESSION_REPLAY_SAMPLE_RATE_KEY] as? Long and I guess the result should be the same? same for the lines below.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh yeah, of course, was a left over from previous implementation

...on-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/SessionReplayFeature.kt Outdated
private val sampleRate: Float,
private val rateBasedSampler: Sampler,
Copy link
Member

Choose a reason for hiding this comment

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

it is kind of duplication of the same. normally we should be able to get sample rate from rateBasedSampler.getSampleRate

Copy link
Member Author

Choose a reason for hiding this comment

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

yes but for that I need to cast the sampler to RateBasedSampler and I want to avoid that

Copy link
Member

Choose a reason for hiding this comment

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

No, getSampleRate is a method of Sampler interface, no case is needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh ...didn't check that, good one ;)

@mariusc83 mariusc83 force-pushed the mconstantin/rum-886/report-sr-in-configuration-telemetry branch from 8cc72eb to 5416510 Compare September 12, 2023 13:37
@mariusc83 mariusc83 requested a review from 0xnm September 12, 2023 14:34
@mariusc83 mariusc83 force-pushed the mconstantin/rum-886/report-sr-in-configuration-telemetry branch from 5416510 to 2f00733 Compare September 12, 2023 14:35
@mariusc83 mariusc83 requested a review from xgouchet September 12, 2023 14:57
@mariusc83 mariusc83 force-pushed the mconstantin/rum-886/report-sr-in-configuration-telemetry branch from 2f00733 to c0a7f5a Compare September 12, 2023 14:58
@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2023 •
edited
Loading

Codecov Report

Merging #1611 (c0a7f5a) into develop (d8b3962) will increase coverage by 0.05%.
The diff coverage is 93.33%.

@@             Coverage Diff             @@
##           develop    #1611      +/-   ##
===========================================
+ Coverage    83.63%   83.68%   +0.05%     
===========================================
  Files          451      451              
  Lines        15567    15557      -10     
  Branches      2318     2317       -1     
===========================================
  Hits         13018    13018              
+ Misses        1930     1915      -15     
- Partials       619      624       +5
Files Changed Coverage Δ
...oid/sessionreplay/internal/SessionReplayFeature.kt 99.02% <80.00%> (-0.98%) ⬇️
...ndroid/telemetry/internal/TelemetryEventHandler.kt 78.18% <100.00%> (-0.66%) ⬇️

... and 26 files with indirect coverage changes

@mariusc83 mariusc83 merged commit ac9865c into develop Sep 12, 2023
@mariusc83 mariusc83 deleted the mconstantin/rum-886/report-sr-in-configuration-telemetry branch September 12, 2023 18:06
@xgouchet xgouchet added this to the 2.2.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xnm 0xnm 0xnm approved these changes

@xgouchet xgouchet xgouchet approved these changes

Assignees

@mariusc83 mariusc83

Labels
None yet
Projects
None yet
Milestone
2.2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@mariusc83 @codecov-commenter @xgouchet @0xnm