-
Notifications
You must be signed in to change notification settings - Fork 20
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
[FSSDK-9775] Log invalid user attributes #369
Conversation
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.
A suggestion. We can discuss about the options.
OptimizelySDK/Event/EventFactory.cs
Outdated
if (Validator.IsUserAttributeValid(userAttribute)) | ||
{ | ||
var attributeId = config.GetAttributeId(userAttribute.Key); | ||
if (!string.IsNullOrEmpty(attributeId)) |
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.
What about switching 204 and 201, so we only log invalid type attributes for attribute ids defined in datafile.
We do not care about types of not-registered attributes.
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.
Let me know what you think @jaeopt
Looks like I'm going to need to work on the C# test app to pass FSC |
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.
The changes look good!
One clarifying question about if we want to log all non-registered attributes or not.
if (notRegisteredAttributeKeys.Count > 0 && !(logger is null)) | ||
{ | ||
logger.Log(LogLevel.WARN, | ||
$"User attributes: {string.Join(", ", notRegisteredAttributeKeys.ToArray())} are not supported by the datafile and will not be used."); |
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.
Not sure if we want to log all these here. I see some clients have 100+ attributes not all registered to us. Each event will dump a bunch of these messages. Let's discuss.
This was a spike only Jira ticket. Woops. 🤷 |
Summary
Test plan
Issues