fix and improve tests, context management by typotter · Pull Request #2896 · DataDog/dd-sdk-android

typotter · 2025-09-25T08:48:16Z

What does this PR do?

This PR refactors the EvaluationContext class and tests in the Flags feature to address comments in #2886

Motivation

This PR addresses review comments from PR #2886:

Public API Issues: Removed InternalLogger exposure and Builder pattern complexity
SDK Consistency: Changed to Map<String, Any?> to match RUM/Logs patterns
API Spec Compliance: Internal normalization ensures Map<String, String> for network calls
Code Quality: Improved documentation, error handling, and test coverage
Architecture: Clean separation between public (flexible) and internal (normalized) APIs

Additional Notes

Key Architectural Changes:

Public EvaluationContext: Accepts any attribute types for developer convenience
Internal DatadogEvaluationContext: Handles validation, normalization, and API compliance
Dependency Injection: DatadogFlagsProvider now accepts EvaluationsManager for better testability

Testing Improvements:

Added comprehensive attribute normalization tests
Enabled proper unit testing with mock EvaluationsManager
Applied SDK testing conventions (Forge, Mockito patterns)

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)

aleksandr-gringauz · 2025-09-26T07:49:46Z

+         */
+        private fun normalizeValue(key: String, value: Any?, logger: InternalLogger): String? = when (value) {
+            null -> {
+                logger.log(


Question.

I see that we support null as values in attributes: Map<String, Any?>. Then here we log a warning if we encounter null and skip it in normalizeAttributes function. Why don't we just have attributes: Map<String, Any> to express using the type system that we don't actually allow nulls?

Is it a fix for this comment?

If in the future we start supporting nulls, we can return to attributes: Map<String, Any?>, it will be backward-compatible.

Yes, this wider-scoping was in response to that previous comment.

Is it better to have an external surface that is congruent with other Features (Logs, RUM) providing flexibility to the developer, or is it better here to limit the external API to just the types that are supported, providing a more concise API for the developer and eliminating the need for filtering warnings?

Ok, I get it, thanks for the clarification.

It's probably better to ask @0xnm opinion.

I would say that Map<String, Any> is better so that the user can't accidentally put nulls there that will not really be uploaded. Especially if null support will not be added soon. But I don't have a strong opinion here, up to you. No blockers from me.

I think the latter (expressing the limitations in the type system) will be better here, because it will reduce the number of questions/support tickets we receive.

Alignment with other features only makes sense if backend API is the same in terms of handling these "key-value" pairs, but here it is different.

OK. Thanks everyone for the insight.

Parity with iOS implementation of Flags is going to take priority over ergonomics and consistency. We are going to limit the attributes to Map<String, String> and put the conversion on the devs. This limited API can always be expanded if/when we expose new types or eliminate the need to stringifiy.

0xnm · 2025-09-26T09:20:55Z

                internalLogger.log(
-                    InternalLogger.Level.ERROR,
+                    InternalLogger.Level.WARN,
                    InternalLogger.Target.MAINTAINER,


could this log message be useful for user as well?

Would it make sense to dual log a warning to the user and an error to the maintainer here? The maintainer will want to know when the SDK internals fail, but to the user it should be mostly transparent and a warning would be appropriate.

Usually it makes sense to keep both MAINTAINER and USER, because:

MAINTAINER - log happens only in the debug SDK builds (what is published on Maven is release builds)

USER - log happens in both debug+release SDK builds.

So if it is useful for the USER and is actionable somehow from their part (or can help in the investigation with Datadog support), it should be shown to the user.

Thank you for the details on the target mechanisms.

I've combined the log calls into a single ERROR.

Co-authored-by: Nikita Ogorodnikov <[email protected]>

codecov-commenter · 2025-09-26T23:01:40Z

Codecov Report

❌ Patch coverage is 88.52459% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.74%. Comparing base (853fd91) to head (71c18f2).

Files with missing lines	Patch %	Lines
...lags/internal/repository/DefaultFlagsRepository.kt	0.00%	3 Missing ⚠️
...ureflags/internal/evaluation/EvaluationsManager.kt	87.50%	2 Missing ⚠️
...d/flags/featureflags/internal/NoOpFlagsProvider.kt	0.00%	1 Missing ⚠️
...ernal/repository/net/DefaultFlagsNetworkManager.kt	50.00%	1 Missing ⚠️

Additional details and impacted files

@@                     Coverage Diff                      @@
##           feature/feature-flagging    #2896      +/-   ##
============================================================
+ Coverage                     70.54%   70.74%   +0.20%     
============================================================
  Files                           828      826       -2     
  Lines                         30123    30078      -45     
  Branches                       5012     5009       -3     
============================================================
+ Hits                          21250    21277      +27     
+ Misses                         7502     7410      -92     
- Partials                       1371     1391      +20

Files with missing lines	Coverage Δ
...src/main/kotlin/com/datadog/android/flags/Flags.kt	`89.80% <100.00%> (+3.31%)`	⬆️
...lags/featureflags/internal/DatadogFlagsProvider.kt	`83.33% <100.00%> (-6.46%)`	⬇️
.../flags/featureflags/internal/model/FlagsContext.kt	`100.00% <100.00%> (ø)`
...reflags/internal/repository/net/EndpointsHelper.kt	`60.98% <100.00%> (-14.63%)`	⬇️
...roid/flags/featureflags/model/EvaluationContext.kt	`100.00% <100.00%> (ø)`
...d/flags/featureflags/internal/NoOpFlagsProvider.kt	`85.71% <0.00%> (ø)`
...ernal/repository/net/DefaultFlagsNetworkManager.kt	`50.00% <50.00%> (+41.41%)`	⬆️
...ureflags/internal/evaluation/EvaluationsManager.kt	`89.29% <87.50%> (+66.03%)`	⬆️
...lags/internal/repository/DefaultFlagsRepository.kt	`43.48% <0.00%> (+28.09%)`	⬆️

... and 28 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

typotter

Thank you for the reviews! PTAL

typotter · 2025-09-26T22:20:16Z

                internalLogger.log(
-                    InternalLogger.Level.ERROR,
+                    InternalLogger.Level.WARN,
                    InternalLogger.Target.MAINTAINER,


Would it make sense to dual log a warning to the user and an error to the maintainer here? The maintainer will want to know when the SDK internals fail, but to the user it should be mostly transparent and a warning would be appropriate.

typotter

Thanks again. ptal

typotter · 2025-10-02T14:59:25Z

                internalLogger.log(
-                    InternalLogger.Level.ERROR,
+                    InternalLogger.Level.WARN,
                    InternalLogger.Target.MAINTAINER,


Thank you for the details on the target mechanisms.

I've combined the log calls into a single ERROR.

typotter · 2025-10-02T16:44:42Z

+         */
+        private fun normalizeValue(key: String, value: Any?, logger: InternalLogger): String? = when (value) {
+            null -> {
+                logger.log(


OK. Thanks everyone for the insight.

Parity with iOS implementation of Flags is going to take priority over ergonomics and consistency. We are going to limit the attributes to Map<String, String> and put the conversion on the devs. This limited API can always be expanded if/when we expose new types or eliminate the need to stringifiy.

0xnm · 2025-10-03T14:39:55Z

                InternalLogger.Level.ERROR,
                InternalLogger.Target.MAINTAINER,
-                { "Unable to find host for site ${flagsContext.site}; we will retry later." },
+                { "Unable to find host for site ${flagsContext.site.name}; we will retry later." },


very minor: it is better to use .siteName here instead of .name, because it can be that build is obfuscated, so .name will print enum item name, which will be obfuscated unlike .siteName string literal.

very helpful catch.

hmm. siteName is internal, so I've changed to use the intakeEndpoint for now.

typotter · 2025-10-03T15:46:12Z

/merge

dd-devflow-routing-codex · 2025-10-03T15:46:17Z

View all feedbacks in Devflow UI.

2025-10-03 15:46:16 UTC ℹ️ Start processing command /merge

2025-10-03 15:46:23 UTC ℹ️ MergeQueue: waiting for PR to be ready

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.

2025-10-03 16:42:18 UTC ℹ️ MergeQueue: merge request added to the queue

The expected merge time in feature/feature-flagging is approximately 0s (p90).

2025-10-03 17:48:17 UTC ℹ️ MergeQueue: This merge request was merged

typotter added 3 commits September 23, 2025 14:45

fix comment detekt

0d76fa1

Fix the tests, improve code quality

acd2e60

more test polish

f96c55e

typotter mentioned this pull request Sep 25, 2025

feat: context management #2886

Merged

lint

f3214d3

typotter changed the title ~~Typo/fixes~~ fix and improve tests, context management Sep 25, 2025

remove net mgr test for now

0a6acf9

typotter marked this pull request as ready for review September 25, 2025 19:08

typotter requested review from a team as code owners September 25, 2025 19:08

typotter requested review from 0xnm and jonathanmos September 25, 2025 19:08

typotter mentioned this pull request Sep 26, 2025

chore: Refactor FlagsProvider and FlagsClient into one #2899

Merged

3 tasks

aleksandr-gringauz reviewed Sep 26, 2025

View reviewed changes

Comment thread ...dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/featureflags/FlagsProvider.kt

aleksandr-gringauz reviewed Sep 26, 2025

View reviewed changes

Comment thread .../kotlin/com/datadog/android/flags/featureflags/internal/repository/DefaultFlagsRepository.kt Outdated

0xnm requested changes Sep 26, 2025

View reviewed changes

Apply suggestions from code review

81079c9

Co-authored-by: Nikita Ogorodnikov <[email protected]>

jonathanmos reviewed Sep 29, 2025

View reviewed changes

Comment thread ...com/datadog/android/flags/featureflags/internal/repository/net/DefaultFlagsNetworkManager.kt Outdated

Comment thread ...ain/kotlin/com/datadog/android/flags/featureflags/internal/repository/net/EndpointsHelper.kt Outdated

address comments

31245f5

typotter requested a review from 0xnm October 1, 2025 19:22

fix line length

5124787

typotter commented Oct 1, 2025

View reviewed changes

typotter requested a review from aleksandr-gringauz October 1, 2025 19:34

typotter added 5 commits October 1, 2025 13:48

added test coverage

5964604

remove unnecessary nullchecks

4103b57

tests

b95aa65

target user and dev in network error log

8d65eff

limit evaluation context to strings, simplify classes

b9defe0

typotter commented Oct 2, 2025

View reviewed changes

aleksandr-gringauz previously approved these changes Oct 3, 2025

View reviewed changes

0xnm previously approved these changes Oct 3, 2025

View reviewed changes

fixes

71c18f2

typotter dismissed stale reviews from 0xnm and aleksandr-gringauz via 71c18f2 October 3, 2025 15:35

0xnm approved these changes Oct 3, 2025

View reviewed changes

dd-devflow Bot added mergequeue-status: waiting mergequeue-status: queued mergequeue-status: in_progress and removed mergequeue-status: waiting mergequeue-status: queued labels Oct 3, 2025

dd-mergequeue Bot merged commit f2fed1b into feature/feature-flagging Oct 3, 2025
26 checks passed

dd-devflow Bot added mergequeue-status: done and removed mergequeue-status: in_progress labels Oct 3, 2025

dd-mergequeue Bot deleted the typo/fixes branch October 3, 2025 17:48

This was referenced Nov 4, 2025

Flags Feature #2979

Closed

Flags Feature #2982

Merged

0xnm restored the typo/fixes branch March 26, 2026 10:46

Uh oh!

Conversation

typotter commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review checklist (to be filled by reviewers)

Uh oh!

aleksandr-gringauz Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

typotter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

typotter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

typotter commented Oct 3, 2025

Uh oh!

dd-devflow-routing-codex Bot commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

typotter commented Sep 25, 2025 •

edited

Loading

aleksandr-gringauz Sep 26, 2025 •

edited

Loading

codecov-commenter commented Sep 26, 2025 •

edited

Loading

dd-devflow-routing-codex Bot commented Oct 3, 2025 •

edited

Loading