DataDog / dd-sdk-ios

Datadog SDK for iOS - Swift and Objective-C.
Apache License 2.0
206 stars 123 forks source link

Use `[String: Any]` attributes instead of `[String: Encodable]`? #1422

Open crewshin opened 1 year ago

crewshin commented 1 year ago

I have to admit the fact that this library implements AttributeKey and AttributeValue is a bit confusing. Why not just accept a [String: Any] and if the end user happens to pass in something other than a basic data type that you can understand... ignore it server side? The any aspect of this whole thing is blowing me up from the typealias' existential jazz.

In my app, I want to pass in [String: Any]'s of data as attributes. I have a lot of data that I want to log which spreads across data types and namespaces.

Simple [String: Any] data I would like to send:

{
    "status_code": 200,
    "url": "https://foo.bar",
    "device": {
        "device_model": "iPhone 14 Pro",
        "screen_brightness": 50.0,
        "device_name": "iPhone",
        "battery_level": -100.0,
        "is_jailbroken": false,
        ...
    },
    "headers": {
        "X-ClientId": "000",
        "X-Platform": "iOS",
        ...
    },
    "global": {
        "bundle_id": "com.foo.bar",
        ...
    },
    "foo": {
        "role": "foo",
        "family_list": [
            {
                "Family_id": 0,
                "Family_name": "foo"
            }
        ],
        ...
    }
}

We have a simple wrapper class to organize a bit of the data we want to pass in. This looping logic is purely here to type cast and maintain the object hierarchy. foo: family_list, etc image

Which uses this type caster func just to build the expected dictionary: image

And sends data to: image

I can pass in any dictionary I want and cast to AttributeValue easily... as long as the data types are not Any. So [String: String], [String: Double], [String: Bool]... they all work great, but as soon as I want to use a [String: Any], things fail.

In the above json example, "device" has mixed value types. As does "foo".

This seems to be the issue here: https://stackoverflow.com/a/76924488/2966596

I'm either A: missing something basic here that makes life easy for a developer. Or B: the developers of this library haven't used this in production or on anything other than unit tests with simple mocked data. Please tell me it's A.

I also hope the solution isn't to create an Encodable struct for everything I want to use.

ncreated commented 1 year ago

Hello @crewshin 👋. The choice of [String: Encodable] is to constraint custom attributes to only ones that can be encoded to an external representation (we use JSON) that can be later understood by our backend. If using [String: Any] we wouldn't be able to guarantee that all your attributes can be encoded → so also sent.

Then, AttributeKey and AttributeValue are only typealiases (to String and to Encodable), standing for documenting purpose. There are certain constraints on how many attribute levels can be nested, explained here.

The problem you're hitting can be solved in a generic way with type erasures, not requiring so much hassle. To get some idea, I recommend you to look at our generic implementation of:

and the AnyEncodable type-erasure that empowers it. Our implementation was inspired with even more versatile AnyCodable project that I also recommend you to look at. Both things use open license, so feel free to utilise the code.

Last resort, you can use our Objective-C SDK, which uses the notion of [String: Any] attributes, but that would be an overkill for Swift project in my opinion: https://github.com/DataDog/dd-sdk-ios/blob/12a92d304aa143020cbae54c4e8622c210207792/DatadogObjc/Sources/Logs/Logs%2Bobjc.swift#L219

I hope this answers the question.

crewshin commented 1 year ago

Hey thanks for the thoughtful response @ncreated!

I understand that you would like to have a guarantee that you can decode the incoming data. However you have chosen to offload all this work to the end user. I would think the scenario I hit is almost certainly the most common one for developers using this library. Why not make it easier for them to work with? This feels like it was built for DD and not with the end user in mind.

I'd still recommend one of the following:

  1. Remove the typealias' usage and allow end users to pass in [String: Any] knowing that they could potentially pass in something that won't work with the backend. Add it to the documentation that they need to handle all data types locally before calling the api.

Or

  1. Change the access control on internal typealias AnyEncodable = DDAnyEncodable from internal to public and add ObjIntercompatibility.swift to the library. Document it's usage.

Remember that onboarding new users should be (imo) as easy as possible and I almost bailed on using this service because it was "too difficult" to get a simple dictionary passed into the API. I have a feeling I'm not alone on this.

So instead of just passing in a simple dictionary, my workaround was to copy those two files mentioned above into my project and maintain ownership. None of which seems to be documented anywhere.

Random note: The Android lib didn't seem to have this requirement and is inconsistent.

Thanks again for the help. I appreciate your time.

ncreated commented 1 year ago

@crewshin Again, having Encodable as value requirement is to leverage in-build Swift type safety. Our Android SDK doesn't use it, because there is no similar thing in Kotlin.

I would think the scenario I hit is almost certainly the most common one for developers using this library. Why not make it easier for them to work with?

AFAIK, it's the first feedback we hear on this. We will consider it a feature request, so we can track how much it is popular - I'll tag this issue accordingly.

goodones-mac commented 12 months ago

Because [String: Encodable] is not encodable conformant itself, I've had to go through many, many contortions in my codebase to be able to encode something equivalent to [String : [String: [String: Encodable]]] in my codebase. Like the output of MetricKit's dictionaryRepresentation(). jsonRepsentation() returns a Data blob, and putting it directly into datadog makes the web interface show a list of numbers vs. decoding it as json.

And it's not that easy to wrap it into an AnyEncodable wrapper that you find on stack overflow. I ran into a lot of edge cases and issues that led me to making custom encodable types for specific dictionary structures vs. dealing with that pain. It's a giant pain the ass to do anything involving a deep dictionary structure currently with datadog. Now with the AnyCodable library that you referenced here, I can finally put recursive dictionaries with that more full featured library without much pain. It might as well be an official feature of the library at this point.

maciejburda commented 5 months ago

Just a quick heads-up that we've closed the GitHub issue you reported due to inactivity. If you've updated your SDK to the latest version and the problem still persists, feel free to open a new issue and we'll be happy to help out.

crewshin commented 5 months ago

@maciejburda Was anything related to this changed in the SDK? Or just closed because old?

maciejburda commented 5 months ago

@crewshin Thanks for checking!

I closed this issue as a part of the clean up. GH Issues are becoming quite busy - being a mix of feature requests, bug reports, configuration issues and questions. This together with our internal channels make things difficult to track, both to us and the customers. I highly recommend sending feature request through the official form.

Although, after second look I agree with @ncreated. Let's keep it open and see if we can get more feedback on this matter, so we can prioritise in the future.

shagedorn commented 2 weeks ago

I just found this thread and would like to leave a +1. While I don't feel it's an absolute necessity (we also wrote our own abstractions to make working with attributes easier), it would definitely be appreciated if the SDK provided helpers that allow sending nested data/JSON more easily. I think I prefer a public conversion helper function over [String: Any] so things don't just fail silently, but don't feel strongly about this.