feat(NODE-4696): include FAAS metadata in the mongodb handshake #3616
Conversation
baileympearson
force-pushed
the
NODE-4696-faas-metadata-handshake-5x
branch
from
0066417 to
c33c171
Compare
baileympearson
force-pushed
the
NODE-4696-faas-metadata-handshake-5x
branch
from
9eb3d34 to
c623356
Compare
baileympearson
changed the title
nbbeeken
added
the
Primary Review
| const numberOfProvidersPresent = [awsPresent, azurePresent, gcpPresent, vercelPresent].filter( | ||
| identity | ||
| ).length; |
There was a problem hiding this comment.
Could this be an ifelse sequence instead?, we have the order encoded below with the early returns, I'd just add an else case to the end of that and return none.
There was a problem hiding this comment.
No, look closely at the logic. It purposefully returns none when there's more than one faas provider present
| gcp: applyGCPMetadata, | ||
| azure: applyAzureMetadata, | ||
| vercel: applyVercelMetadata, | ||
| none: identity |
There was a problem hiding this comment.
instead of adding a step for none, can't none do nothing to the input? maybe none could instead be represented by a null determineFAASProvider result that leads to returning the metadata input.
There was a problem hiding this comment.
none does do nothing to the input, that's what identity is. I chose this over using null for simplicity in typing and edge casing. It flows everything through the same path logically
There was a problem hiding this comment.
an early return based on null should narrow the key to be one of the acceptable strings, this way we can write less handling for the obvious case.
There was a problem hiding this comment.
That still requires handling for the obvious case, no? it's just that you've put it in a null check. I like this approach because there's no special casing values (or a "lack" of value here, indicated by none or null)
| export function identity<T>(obj: T): T { | ||
| return obj; | ||
| } |
There was a problem hiding this comment.
What does this provide that an inline arrow function does not? (Also I have suggestions that make this unnecessary)
There was a problem hiding this comment.
Typing and naming. It's used in two places
There was a problem hiding this comment.
Doesn't an arrow function provide the same type information? x => x should return the input type right?
There was a problem hiding this comment.
id or identity is commonly builtin with the semantics I provided. For example:
- http://www.lispworks.com/documentation/lw70/CLHS/Body/f_identi.htm#:~:text=identity%20is%20intended%20for%20use,is%20a%20number%20or%20character.
- https://clojuredocs.org/clojure.core/identity
- http://zvon.org/other/haskell/Outputprelude/id_f.html
I can remove it though if we want
nbbeeken
changed the title
Description
What is changing?
When present, FAAS metadata is included in the mongodb handshake.
Also included:
utilsfile.MongoOptionshas a new option,truncatedClientMetadata, to preserve the full metadata on themetadataproperty while still only truncating the metadata once. The driver now uses thetruncatedMetadatafield internally.Is there new documentation needed for these changes?
No.
What is the motivation for this change?
Double check the following
npm run check:lintscripttype(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript