Skip to content
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

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

Open
crewshin opened this issue Aug 17, 2023 · 7 comments
Open

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

crewshin opened this issue Aug 17, 2023 · 7 comments
Assignees

Comments

@crewshin
Copy link

crewshin commented Aug 17, 2023

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 ncreated self-assigned this Aug 18, 2023
@ncreated ncreated added the question Further information is requested label Aug 18, 2023
@ncreated
Copy link
Collaborator

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:

public func debug(_ message: String, attributes: [String: Any]) {

I hope this answers the question.

@ncreated ncreated changed the title Why does this sdk implement AttributeKey and AttributeValue? Why not [String: Any] Why [AttributeKey: AttributeValue] not [String: Any]? Aug 18, 2023
@crewshin
Copy link
Author

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
Copy link
Collaborator

ncreated commented Sep 4, 2023

@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.

@ncreated ncreated changed the title Why [AttributeKey: AttributeValue] not [String: Any]? Use [String: Any] attributes instead of [String: Encodable]? Sep 4, 2023
@ncreated ncreated added feature and removed question Further information is requested labels Sep 4, 2023
@goodones-mac
Copy link

goodones-mac commented Oct 12, 2023

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
Copy link
Contributor

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
Copy link
Author

crewshin commented May 9, 2024

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

@maciejburda
Copy link
Contributor

maciejburda commented May 9, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants