DataDog
Type Mismatch in `User` interface for Datadog Tracer
Expected behaviour
The setUser method in the Datadog Tracer library should accept user attributes of various types including number, boolean, and Date, as evident from actual usage and logs.
While this exact payload it's completely valid if bypassed by @ts-ignore, type casting or any other method.
Actual behaviour
The TypeScript definition for the User interface restricts attributes to be of type string. This is inconsistent with actual behavior and leads to unnecessary type errors during development.
Steps to reproduce
Try using the setUser method with user attributes that are not of type string.
tracer.setUser({
id: 1, // number
email: "[email protected]", // string
createdAt: new Date(), // Date object
isEnabled: true, // boolean
});
// TypeScript will throw a type error.
So after digging a bit, I can give you a bit more context on this.
Our architecture only allows us to ingest strings for user data. The reason why it works when you ignore the typescript warning is because we force string casting when serializing the user object here: https://github.com/DataDog/dd-trace-js/blob/c452005127a99285dfd95affeec64a4d1c1769c4/packages/dd-trace/src/appsec/sdk/set_user.js#L8 This is only present as a best effort safety measure. We are technically accepting anything that can be stringified, but we'd rather incite the library user to stringify the data by himself, instead of being forcefully stringified by us, potentially in an undesirable way.
As of now, if you want to pass arbitrary data to the setUser() method, I'd encourage you to stringify your data yourself before passing it, instead of relying on our safety measure and a @ts-ignore, as this is not officially supported by dd-trace and will potentially break without warning.
We could I guess add support for passing any value and delegate forced stringification to JSON.stringify() for example, but this would be a feature request, and a modification of the SDK public API.
So after digging a bit, I can give you a bit more context on this.
Our architecture only allows us to ingest strings for user data. The reason why it works when you ignore the typescript warning is because we force string casting when serializing the user object here:
https://github.com/DataDog/dd-trace-js/blob/c452005127a99285dfd95affeec64a4d1c1769c4/packages/dd-trace/src/appsec/sdk/set_user.js#L8
This is only present as a best effort safety measure. We are technically accepting anything that can be stringified, but we'd rather incite the library user to stringify the data by himself, instead of being forcefully stringified by us, potentially in an undesirable way. As of now, if you want to pass arbitrary data to the
setUser()method, I'd encourage you to stringify your data yourself before passing it, instead of relying on our safety measure and a@ts-ignore, as this is not officially supported bydd-traceand will potentially break without warning.We could I guess add support for passing any value and delegate forced stringification to
JSON.stringify()for example, but this would be a feature request, and a modification of the SDK public API.
Hey @simon-id first of all thanks for the quick response, while I agree with some points, I don't think this should be a feature request, heres why:
Inconsistency Between Implementation and Type Definition: The first argument is that TypeScript is a superset of JavaScript. If the underlying JavaScript library has the capacity to accept various types and then stringifies them internally, then the TypeScript definition should align with that capability. The current TS definition is misleading and not representative of the underlying JavaScript implementation.
Developer Experience: Forcing the developer to stringify the data adds unnecessary friction and reduces code readability. If the library can do it internally without any side-effects, it should. The TypeScript definition should reflect this behavior to improve the dev experience.
'Best-effort Safety Measure' is Misleading: If the library already contains a 'best-effort safety measure' that casts to a string, then the claim of 'potential breakage' is invalid. It can either break or it can't, and if it hasn't, then the library has effectively accepted other types and converted them. The type definition should acknowledge this fact.
Official Support vs Community Use: The library might not officially support types other than string, but if it's being used that way in the community without issue, the type definition should adapt to the practical use-cases rather than being rigidly theoretical. Isn't the end goal for the library to be useful and user-friendly?
Contradiction in Approach: If you suggest using JSON.stringify() for forced stringification, then you acknowledges that stringification is a valid operation that the library should perform. If that's the case, why not let TypeScript reflect the true capabilities of the library? Why have a type definition that doesn't actually define the type you'll get?
Clear Communication: If the library has technical limitations (only strings can be ingested at the backend), this should be documented. However, the TypeScript definition should not serve as this documentation; it should describe the API as it can be used.
@Paw-liblab sorry about the late reply, we had an unusual couple of weeks in the team.
f the underlying JavaScript library has the capacity to accept various types and then stringifies them internally, then the TypeScript definition should align with that capability.
The way I see it, the typings and the API documentation are a contract. If you're willing to break that contract, I don't really mind, but I can't guarantee I'll support your use case if you try. Now the API contract we decided on is "strings only" because the responsibility of stringifying values ourselves while supporting every use case is a responsibility I'm not willing to take. It doesn't matter what's the underlying internals.
Forcing the developer to stringify the data adds unnecessary friction and reduces code readability
It also forces the user to do things correctly instead of relying on our weak safety net. It's a tradeoff between safety and comfort.
If the library already contains a 'best-effort safety measure' that casts to a string, then the claim of 'potential breakage' is invalid.
This safety net is not advertised in the public API contract.
Your argument is weird imo. For example nothing prevents you from using dd-trace's internals, and use features not in the public API. But these might change without any notice and break your code because they're not part of the public API contract. Same goes for non strings values passed to setUser().
The library might not officially support types other than string, but if it's being used that way in the community without issue
Who is that "community" you're referring to ? You're the first I hear complaining about this. If this become a widespread concern, then obviously we'll look into it more seriously.
the type definition should adapt to the practical use-cases rather than being rigidly theoretical.
That would force me to implement a feature that is difficult to maintain, and I'm not willing to do that for such a small benefit.
If you suggest using JSON.stringify() for forced stringification, then you acknowledges that stringification is a valid operation that the library should perform.
Non sequitur.
If the library has technical limitations (only strings can be ingested at the backend), this should be documented.
I agree that we have subpar documentation and I've been trying to prioritize working on that. :+1:
Unless you have new arguments, this will probably be my final answer.
I'm closing this issue and the PR with it as there hasn't been any activity or other customer request about this topic.