[sc-170667] Add basic logging support. #4
Conversation
|
This pull request has been linked to Shortcut Story #170667: Add logging to the node OpenFeature provider.. |
| @@ -0,0 +1,67 @@ | |||
| import { LDLogger } from 'launchdarkly-node-server-sdk'; | |||
There was a problem hiding this comment.
This is mostly a duplicate of the js-server-sdk. Possibly that could be exported in that version, so we wouldn't duplicate it.
The OpenFeature sdk also has basically the same code in it. Also not exported.
There was a problem hiding this comment.
I'd rather not export it in the SDK module, because it's an implementation detail that we'd rather not have to maintain without breaking changes for application use. Exporting it from a common package would be a different matter, since that stuff can be versioned separately and is explicitly for our own consumption.
| addCustom(convertedContext, key, value as boolean[]); | ||
| } else if (value.every((val) => typeof val === 'number')) { | ||
| addCustom(convertedContext, key, value as string[]); | ||
| } else { |
There was a problem hiding this comment.
This conversion is a bit more strict, because it matched the actual .d.ts. Hopefully we don't have this long though, as U2C does support these types.
There was a problem hiding this comment.
Hmm... I don't think it was ever correct to say that a custom attribute that's an array must have every element be the same type. I mean, the LD data model and evaluation algorithm had no such restriction. I think that may have been a misunderstanding based on the fact that some older SDK versions (I'm thinking of Java) had custom attribute setters for "array of booleans", etc. because we thought those would be common use cases.
There was a problem hiding this comment.
I will adjust this a bit.
There was a problem hiding this comment.
That is— an LD user whose JSON representation is {"key": "a", "custom": {"b": [1, "yes", true]}} is a valid user, and every implementation of the LD evaluation engine, as far as I know, will let you evaluate a clause like {"attribute": "b", "op": "in", "values": [2, "yes"]} against that user. Mixing types doesn't hurt anything, it just means that any operators that can't be applied to the type of one of the elements will be a non-match for that element. In my example, it would be a match because there is at least one element value in the user attribute ("yes") that is a match for one of the values in the clause.
There was a problem hiding this comment.
When I had put the check in earlier I had put it in wrong for whatever reason. Now it complies correctly. Which is that it is an array of that type union. Not arrays of each type. Removed the mixed type test as well. It does only allow the types in the .d.ts still. Which is still probably more restrictive than it really has been.
For review after the addition of the releaser config.
Adds basic logging support, and logs in a few places.