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

[FEATURE] Expose TargetingKey with all value exporters #235

Closed
Kavindu-Dodan opened this issue Feb 14, 2024 · 5 comments · Fixed by #287
Closed

[FEATURE] Expose TargetingKey with all value exporters #235

Kavindu-Dodan opened this issue Feb 14, 2024 · 5 comments · Fixed by #287
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@Kavindu-Dodan
Copy link

Requirements

Similar to the Java SDK [1], Dotnet SDK EvaluationContext should include targetingKey with bulk exporters.

Currently EvaluationContext exposes AsDictionary() [2] as well as GetEnumerator()[3], but they do not include the targetingKey, which is stored in a dedicated field.

AC

Include the targetingKey in bulk exporters and refactor of the property to match with this change

[1] - open-feature/java-sdk#801
[2] - https://github.com/open-feature/dotnet-sdk/blob/v1.4.1/src/OpenFeature/Model/EvaluationContext.cs#L76
[3] - https://github.com/open-feature/dotnet-sdk/blob/v1.4.1/src/OpenFeature/Model/EvaluationContext.cs#L90

@Kavindu-Dodan Kavindu-Dodan added the enhancement New feature or request label Feb 14, 2024
@toddbaert toddbaert added good first issue Good for newcomers help wanted Extra attention is needed labels Feb 15, 2024
@toddbaert toddbaert added the contribfest A good issue for Contribfest KubeCon EU '24 label Mar 11, 2024
@kinyoklion
Copy link
Member

The addition of the TargetingKey property has presented some problems for the LaunchDarkly provider. We had supported using attributes with the name "key" (convenient coming from an LD context) as well as "targetingKey". The TargetingKey not being in the map just meant we basically were unaware of it.

I wonder if we should have just made it short-hand for "targetingKey" as a member of the underlying structure. If we just export it, then we have to use some kind of name that will shadow whatever we pick. (Likely "targetingKey").

Instead we could adjust the general Set method to have restrictions on "targetingKey". We do something similar in LD contexts where if you call the set function with a built-in attribute we do our best, and then log if we cannot handle it.

Similar situation: https://github.com/launchdarkly/dotnet-sdk-common/blob/d248d74b29fb0d1e27771c5810b8277fd256e266/src/LaunchDarkly.CommonSdk/ContextBuilder.cs#L272

@toddbaert
Copy link
Member

toddbaert commented Jul 4, 2024

@kinyoklion I think I understand what you're suggesting, but I want to be sure... Are you suggesting that you think the targeting key setter/getter should just retrieve a "special" property from the map (probably targetingKey?

I think that's fine, if so. I think the real problem is just that people will expect the AsDictionary() and GetEnumerator() to convey a targeting key, somehow if it's set.

We're close to being about to release a 2.0, so I'm reviewing some of these open issues.

@toddbaert toddbaert removed the contribfest A good issue for Contribfest KubeCon EU '24 label Jul 4, 2024
@kinyoklion
Copy link
Member

@toddbaert Yeah, I am suggesting that the key of "targetingKey" and the setter/getter for targetingKey should operate on the same value.

Which either requires a specially interacted with field or a special entry in the map with specialized logic for the set/get generic mechanisms.

Set("targetingKey", "potato");
TargetingKey = "potato";

var targetingKey = Get("targetingKey");
var targetingKey = TargetingKey

Should all basically be setting/getting the same field, and that field should also be in the dictionary.

@toddbaert
Copy link
Member

I'm fine with that. I'll add it to the description, and try to get to it myself if nobody else does first.

@toddbaert toddbaert changed the title [FEATURE] Consider exposing TargetingKey with all value exporters [FEATURE] Expoxe TargetingKey with all value exporters Jul 8, 2024
@toddbaert toddbaert self-assigned this Jul 23, 2024
@toddbaert
Copy link
Member

I'm starting on this.

@benjiro benjiro changed the title [FEATURE] Expoxe TargetingKey with all value exporters [FEATURE] Expose TargetingKey with all value exporters Jul 25, 2024
arttonoyan pushed a commit to arttonoyan/dotnet-sdk that referenced this issue Oct 16, 2024
Use the internal dictionary for the `targetingKey`.

This is non-breaking from a compiler perspective. It could result in
some behavioral changes, but IMO they are largely desirable.

Fixes: open-feature#235

---------

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: André Silva <2493377+askpt@users.noreply.github.com>
Signed-off-by: Artyom Tonoyan <artonoyan@servicetitan.com>
arttonoyan pushed a commit to arttonoyan/dotnet-sdk that referenced this issue Nov 17, 2024
Use the internal dictionary for the `targetingKey`.

This is non-breaking from a compiler perspective. It could result in
some behavioral changes, but IMO they are largely desirable.

Fixes: open-feature#235

---------

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: André Silva <2493377+askpt@users.noreply.github.com>
Signed-off-by: Artyom Tonoyan <artonoyan@servicetitan.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants