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-2885 Add addFeatureFlagEvaluation function for RUM #1265

Merged
merged 2 commits into from
Feb 7, 2023

Conversation

fuzzybinary
Copy link
Member

What does this PR do?

addFeatureFlagEvalutaion adds the result of evaluating a feature flag to a view, which is key / value pair. These feature flags are sent with any subsequent View updates, as well as on Errors until the view is stopped.

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)

`addFeatureFlagEvalutaion` adds the result of evaluating a feature flag to a view, which is key / value pair. These feature flags are sent with any subsequent View updates, as well as on Errors until the view is stopped.
@fuzzybinary fuzzybinary requested a review from a team as a code owner February 3, 2023 22:21
@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2023

Codecov Report

Merging #1265 (25fa963) into develop (31b0121) will decrease coverage by 0.11%.
The diff coverage is 66.67%.

@@             Coverage Diff             @@
##           develop    #1265      +/-   ##
===========================================
- Coverage    82.01%   81.90%   -0.11%     
===========================================
  Files          361      361              
  Lines        12832    12847      +15     
  Branches      2149     2151       +2     
===========================================
- Hits         10524    10522       -2     
- Misses        1655     1667      +12     
- Partials       653      658       +5     
Impacted Files Coverage Δ
.../main/kotlin/com/datadog/android/rum/RumMonitor.kt 78.85% <ø> (ø)
.../android/rum/internal/monitor/DatadogRumMonitor.kt 89.42% <0.00%> (-1.93%) ⬇️
...g/android/rum/internal/domain/scope/RumRawEvent.kt 82.50% <80.00%> (-0.08%) ⬇️
.../android/rum/internal/domain/scope/RumViewScope.kt 95.50% <100.00%> (+0.05%) ⬆️
.../datadog/android/sessionreplay/utils/ThemeUtils.kt 83.33% <0.00%> (-16.67%) ⬇️
...id/webview/internal/rum/WebViewRumEventConsumer.kt 73.53% <0.00%> (-7.35%) ⬇️
...droid/rum/tracking/FragmentViewTrackingStrategy.kt 75.00% <0.00%> (-3.85%) ⬇️
...ndroid/telemetry/internal/TelemetryEventHandler.kt 70.31% <0.00%> (-2.34%) ⬇️
...al/persistence/file/batch/BatchFileOrchestrator.kt 93.70% <0.00%> (-1.57%) ⬇️
...ain/java/com/datadog/opentracing/PendingTrace.java 59.48% <0.00%> (-0.86%) ⬇️
... and 4 more

Copy link
Member

@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've added some minor comments to the tests

@StringForgery flagName: String,
@StringForgery flagValue: String
) {
// GIVEN
Copy link
Member

Choose a reason for hiding this comment

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

Empty GIVEN. Should it be like that?

Copy link
Member Author

Choose a reason for hiding this comment

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

All the givens here are set up by the fixture setup. I'm not sure if I should keep the comment and have a blank body or remove the comment?

Copy link
Member

Choose a reason for hiding this comment

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

You can just remove it I think. They are all added manually, we don't have a template.

argumentCaptor<ViewEvent> {
verify(mockWriter).write(eq(mockEventBatchWriter), capture())
assertThat(lastValue)
.apply {
Copy link
Member

Choose a reason for hiding this comment

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

apply is not needed here if I'm not wrong, direct call is possible. Same for all tests 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.

👍 Yeah, I guess I only need apply if I were checking multiple things.

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 not needed even for multiple assertions, because each assertion returns Assert instance, it can be called in the chain.

}

// endregion

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps missing a test on not sending pre-existing Feature Flag after a new view is started.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'll probably need to see if I can add that to DatadogRumMonitorTest since that's what would handle changing the scope.

@fuzzybinary fuzzybinary force-pushed the jward/RUMM-2885-feature-flags branch from 88eb385 to 25fa963 Compare February 6, 2023 20:15
@fuzzybinary fuzzybinary requested a review from 0xnm February 7, 2023 13:54
@fuzzybinary
Copy link
Member Author

@0xnm Can I get a review of the new test before I merge in?

Copy link
Member

@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.

looks good!

@fuzzybinary fuzzybinary merged commit a9351e9 into develop Feb 7, 2023
@fuzzybinary fuzzybinary deleted the jward/RUMM-2885-feature-flags branch February 7, 2023 15:16
@xgouchet xgouchet added this to the 1.18.0 milestone Dec 13, 2023
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