Thread allocationKey into ResolutionDetails flagMetadata #3534
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -167,6 +167,7 @@ internal class DatadogFlagsClient( | |
| trackResolution(resolution) | ||
| createSuccessResolution(resolution.flag, resolution.value) | ||
| } | ||
|
|
||
| is InternalResolution.Error -> { | ||
| trackErrorResolution(resolution) | ||
| createErrorResolution( | ||
|
|
@@ -321,6 +322,7 @@ internal class DatadogFlagsClient( | |
| errorCode = ErrorCode.TYPE_MISMATCH | ||
| errorMessage = exception.message ?: "Type mismatch" | ||
| } | ||
|
|
||
| else -> { | ||
| errorCode = ErrorCode.PARSE_ERROR | ||
| val typeName = FlagValueConverter.getTypeName(defaultValue::class) | ||
|
|
@@ -366,6 +368,7 @@ internal class DatadogFlagsClient( | |
| trackResolution(resolution) | ||
| resolution.value | ||
| } | ||
|
|
||
| is InternalResolution.Error -> { | ||
| // Only log type mismatches as warnings to help developers identify configuration issues. | ||
| // Other errors (FLAG_NOT_FOUND, PARSE_ERROR) are expected in normal operation. | ||
|
|
@@ -395,9 +398,23 @@ internal class DatadogFlagsClient( | |
| reason = parseReason(precomputedFlag.reason), | ||
| errorCode = null, | ||
| errorMessage = null, | ||
| flagMetadata = buildMetadata(precomputedFlag) | ||
| ) | ||
|
|
||
| private fun buildMetadata(precomputedFlag: PrecomputedFlag): Map<String, Any> { | ||
| val metadata = mutableMapOf<String, Any>() | ||
| precomputedFlag.extraLogging.keys().forEach { key -> | ||
| val value = precomputedFlag.extraLogging.opt(key) | ||
| when (value) { | ||
| is String, is Number, is Boolean -> metadata[key] = value | ||
| } | ||
| } | ||
| if (precomputedFlag.allocationKey.isNotBlank()) { | ||
| metadata["allocationKey"] = precomputedFlag.allocationKey | ||
| } | ||
| return metadata | ||
| } | ||
|
|
||
| private fun <T : Any> createErrorResolution( | ||
| flagKey: String, | ||
| defaultValue: T, | ||
|
|
@@ -429,22 +446,6 @@ internal class DatadogFlagsClient( | |
| } | ||
| } | ||
|
|
||
| private fun <T : Any> trackResolution(resolution: InternalResolution.Success<T>) { | ||
| trackResolution(resolution.flagKey, resolution.flag, resolution.context) | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -249,7 +249,7 @@ internal class DatadogFlagsClientTest { | |
| targetingKey = forge.anAlphabeticalString(), | ||
| attributes = emptyMap() | ||
| ) | ||
| whenever(mockFlagsRepository.getPrecomputedFlagWithContext(fakeFlagKey)) doReturn null | ||
| whenever(mockFlagsRepository.getEvaluationContext()) doReturn fakeEvaluationContext | ||
|
|
||
| // When | ||
|
|
@@ -273,8 +273,8 @@ internal class DatadogFlagsClientTest { | |
| targetingKey = forge.anAlphabeticalString(), | ||
| attributes = emptyMap() | ||
| ) | ||
| whenever(mockFlagsRepository.getPrecomputedFlagWithContext(fakeFlagKey)) doReturn | ||
| (fakeFlag to fakeEvaluationContext) | ||
|
|
||
| // When | ||
| val result = testedClient.resolveBooleanValue(fakeFlagKey, fakeDefaultValue) | ||
|
|
@@ -803,6 +803,7 @@ internal class DatadogFlagsClientTest { | |
| val fakeDefaultValue = forge.aBool() | ||
| val fakeFlagValue = !fakeDefaultValue | ||
| val fakeVariationKey = forge.anAlphabeticalString() | ||
| val fakeAllocationKey = forge.anAlphabeticalString() | ||
| val fakeReason = forge.anElementFrom("STATIC", "TARGETING_MATCH", "RULE_MATCH", "DEFAULT") | ||
| val fakeExtraLogging = JSONObject().apply { | ||
| put("version", forge.anAlphabeticalString()) | ||
|
|
@@ -813,7 +814,8 @@ internal class DatadogFlagsClientTest { | |
| variationValue = fakeFlagValue.toString(), | ||
| variationKey = fakeVariationKey, | ||
| reason = fakeReason, | ||
| extraLogging = fakeExtraLogging, | ||
| allocationKey = fakeAllocationKey | ||
| ) | ||
| val fakeContext = EvaluationContext( | ||
| targetingKey = forge.anAlphabeticalString(), | ||
|
|
@@ -832,6 +834,119 @@ internal class DatadogFlagsClientTest { | |
| assertThat(result.errorMessage).isNull() | ||
| assertThat(result.flagMetadata).isNotNull | ||
| assertThat(result.flagMetadata).containsKeys("version", "environment") | ||
| assertThat(result.flagMetadata["allocationKey"]).isEqualTo(fakeAllocationKey) | ||
| } | ||
|
|
||
| @Test | ||
| fun `M typed allocationKey wins W resolve() { extraLogging also contains allocationKey }`(forge: Forge) { | ||
| // Given | ||
| val fakeFlagKey = forge.anAlphabeticalString() | ||
| val fakeDefaultValue = forge.aBool() | ||
| val fakeFlagValue = !fakeDefaultValue | ||
| val fakeAllocationKey = forge.anAlphabeticalString() | ||
| val fakeExtraLoggingAllocationKey = forge.anAlphabeticalString() | ||
| val fakeFlag = forge.getForgery<PrecomputedFlag>().copy( | ||
| variationType = VariationType.BOOLEAN.value, | ||
| variationValue = fakeFlagValue.toString(), | ||
| allocationKey = fakeAllocationKey, | ||
| extraLogging = JSONObject().apply { | ||
| put("allocationKey", fakeExtraLoggingAllocationKey) | ||
| } | ||
| ) | ||
| val fakeContext = EvaluationContext( | ||
| targetingKey = forge.anAlphabeticalString(), | ||
| attributes = emptyMap() | ||
| ) | ||
| whenever(mockFlagsRepository.getPrecomputedFlagWithContext(fakeFlagKey)) doReturn (fakeFlag to fakeContext) | ||
|
|
||
| // When | ||
| val result = testedClient.resolve(fakeFlagKey, fakeDefaultValue) | ||
|
|
||
| // Then - typed allocationKey wins over any "allocationKey" entry from extraLogging | ||
| assertThat(result.flagMetadata["allocationKey"]).isEqualTo(fakeAllocationKey) | ||
| } | ||
|
|
||
| @Test | ||
| fun `M allocationKey excluded from metadata W resolve() { empty allocationKey }`(forge: Forge) { | ||
| // Given | ||
| val fakeFlagKey = forge.anAlphabeticalString() | ||
| val fakeDefaultValue = forge.aBool() | ||
| val fakeFlagValue = !fakeDefaultValue | ||
| val fakeFlag = forge.getForgery<PrecomputedFlag>().copy( | ||
| variationType = VariationType.BOOLEAN.value, | ||
| variationValue = fakeFlagValue.toString(), | ||
| allocationKey = "", | ||
| extraLogging = JSONObject() | ||
| ) | ||
| val fakeContext = EvaluationContext( | ||
| targetingKey = forge.anAlphabeticalString(), | ||
| attributes = emptyMap() | ||
| ) | ||
| whenever(mockFlagsRepository.getPrecomputedFlagWithContext(fakeFlagKey)) doReturn (fakeFlag to fakeContext) | ||
|
|
||
| // When | ||
| val result = testedClient.resolve(fakeFlagKey, fakeDefaultValue) | ||
|
|
||
| // Then | ||
| assertThat(result.value).isEqualTo(fakeFlagValue) | ||
| assertThat(result.flagMetadata).doesNotContainKey("allocationKey") | ||
| } | ||
|
|
||
| @Test | ||
| fun `M allocationKey excluded from metadata W resolve() { whitespace allocationKey }`(forge: Forge) { | ||
| // Given | ||
| val fakeFlagKey = forge.anAlphabeticalString() | ||
| val fakeDefaultValue = forge.aBool() | ||
| val fakeFlagValue = !fakeDefaultValue | ||
| val fakeFlag = forge.getForgery<PrecomputedFlag>().copy( | ||
| variationType = VariationType.BOOLEAN.value, | ||
| variationValue = fakeFlagValue.toString(), | ||
| allocationKey = " ", | ||
| extraLogging = JSONObject() | ||
| ) | ||
| val fakeContext = EvaluationContext( | ||
| targetingKey = forge.anAlphabeticalString(), | ||
| attributes = emptyMap() | ||
| ) | ||
| whenever(mockFlagsRepository.getPrecomputedFlagWithContext(fakeFlagKey)) doReturn (fakeFlag to fakeContext) | ||
|
|
||
| // When | ||
| val result = testedClient.resolve(fakeFlagKey, fakeDefaultValue) | ||
|
|
||
| // Then | ||
| assertThat(result.value).isEqualTo(fakeFlagValue) | ||
| assertThat(result.flagMetadata).doesNotContainKey("allocationKey") | ||
| } | ||
|
|
||
| @Test | ||
| fun `M null extraLogging value excluded from metadata W resolve() { extraLogging has null value }`( | ||
| forge: Forge | ||
| ) { | ||
| // Given | ||
| val fakeFlagKey = forge.anAlphabeticalString() | ||
| val fakeDefaultValue = forge.aBool() | ||
| val fakeFlagValue = !fakeDefaultValue | ||
| val fakeValidValue = forge.anAlphabeticalString() | ||
| val fakeFlag = forge.getForgery<PrecomputedFlag>().copy( | ||
| variationType = VariationType.BOOLEAN.value, | ||
| variationValue = fakeFlagValue.toString(), | ||
| extraLogging = JSONObject().apply { | ||
| put("nullKey", JSONObject.NULL) // JSONObject.NULL is not String/Number/Boolean | ||
| put("validKey", fakeValidValue) | ||
| } | ||
| ) | ||
| val fakeContext = EvaluationContext( | ||
| targetingKey = forge.anAlphabeticalString(), | ||
| attributes = emptyMap() | ||
| ) | ||
| whenever(mockFlagsRepository.getPrecomputedFlagWithContext(fakeFlagKey)) doReturn (fakeFlag to fakeContext) | ||
|
|
||
| // When | ||
| val result = testedClient.resolve(fakeFlagKey, fakeDefaultValue) | ||
|
|
||
| // Then - JSONObject.NULL is dropped; only primitive values pass through | ||
| assertThat(result.flagMetadata).doesNotContainKey("nullKey") | ||
| assertThat(result.flagMetadata["validKey"]).isEqualTo(fakeValidValue) | ||
| } | ||
|
|
||
| @Test | ||
|
|
@@ -1250,8 +1365,6 @@ internal class DatadogFlagsClientTest { | |
| attributes = emptyMap() | ||
| ) | ||
|
|
||
| whenever(mockFeatureSdkCore.getFeature(any())) doReturn null | ||
| whenever(mockFlagsRepository.getPrecomputedFlagWithContext(fakeFlagKey)) doReturn | ||
| (fakeFlag to fakeEvaluationContext) | ||
|
|
@@ -1359,9 +1472,6 @@ internal class DatadogFlagsClientTest { | |
| attributes = emptyMap() | ||
| ) | ||
|
|
||
| testedClient = DatadogFlagsClient( | ||
| featureSdkCore = mockFeatureSdkCore, | ||
| evaluationsManager = mockEvaluationsManager, | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: do we not need to worry about Long with builder.putLong? https://github.com/open-feature/kotlin-sdk/blob/cfe7ac44d92aef0366e4e6cb8bd01ce070ea06d7/kotlin-sdk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/FlagEvaluationMetadata.kt#L48