RUMM-886 `setUserInfo(..., extraInfo)` added #315

buranmert · 2020-11-20T17:27:14Z

What and why?

Datadog.UserInfo has arbitrary attributes via extraInfo

How?

UserInfoProvider keeps extraInfo dictionary along with userInfo
Features (Logs, APM and RUM) encode extraInfo according to their own rules

Logs:

extraInfo: ["key": "value"] -> "usr.key: value"

APM:

extraInfo: ["key": "value"] -> "meta.usr.key: value"

RUM:

extraInfo: ["key": "value"] -> "context.usr.key: value"

Discussion 💭

In this PR, I put usr. prefix to extraInfo within UserInfoProvider and pass it to features as regular attributes.
But UserInfoProvider passes UserInfo value without encoding and the features (LogEncoder and SpanEncoder) are responsible for its encoding.
This is some sort of inconsistency in UserInfoProvider, I didn't like that ❌

Can't we make UserInfo: Encodable to de-duplicate its encoding logic?

Review checklist

Feature or bugfix MUST have appropriate tests (unit, integration)
Make sure each commit and the PR mention the Issue number or JIRA reference

ncreated · 2020-11-24T14:26:33Z

In this PR, I put usr. prefix to extraInfo within UserInfoProvider and pass it to features as regular attributes.
But UserInfoProvider passes UserInfo value without encoding and the features (LogEncoder and SpanEncoder) are responsible for its encoding.
This is some sort of inconsistency in UserInfoProvider, I didn't like that ❌

What exactly is inconsistent?

UserInfoProvider provides the UserInfo model and each feature decides on its own, how to encode it:

Logging feature puts them at the root level of the log JSON: usr.id, usr.name, usr.email,
Tracing feature nests them under meta object in the span JSON: meta.usr.id, meta.usr.name, meta.usr.email,
RUM feature uses auto-generated RUMDataUSR model to encode it according to rum-events-format.

Having single struct UserInfo: Encodable would be a wrong abstraction, as each feature needs to decide by its own.

In this PR, I put usr. prefix to extraInfo within UserInfoProvider

In all other places (see: NetworkConnectionInfoProvider, CarrierInfoProvider, UserInfoProvider) the encoding keys of custom data are decided by the encoder (LogEncoder, SpanEncoder). Creating user attribute keys at the beginning of data flow leaks this abstraction. I don't see any benefit of this approach, why do you think it's better?

ncreated

Looks very good 👌, but needs the fix for Tracing.

ncreated · 2020-11-26T09:22:27Z

Sources/Datadog/Datadog.swift

@@ -78,9 +78,15 @@ public class Datadog {
    public static func setUserInfo(


This public API is missing the comment.

ncreated · 2020-11-26T09:49:55Z

Sources/Datadog/Tracing/Span/SpanEncoder.swift

@@ -198,5 +198,8 @@ internal struct SpanEncoder {
            let metaKey = "meta.\($0.key)"
            try container.encode($0.value, forKey: DynamicCodingKey(metaKey))
        }
+        try span.userInfo.extraInfo.forEach {
+            try container.encode(EncodableValue($1), forKey: DynamicCodingKey("meta.usr.\($0)"))


This leads to all spans being rejected by Intake if the user extra attribute is not String, e.g.:

Datadog.setUserInfo( id: "abcd-1234", name: "foo", email: "foo@example.com", extraInfo: [ "is-registered": true ] )

As stated in this method comment:

// NOTE: RUMM-299 only string values are supported for `meta.*` attributes

Whatever we encode on meta.* key must be serialised to String representation. We should use JSONStringEncodableValue in SpanBuilder as it is done for span.tags.

ncreated · 2020-11-26T09:56:21Z

Tests/DatadogTests/Datadog/RUMMonitorTests.swift

+                        id: "abc-123",
+                        name: "Foo",
+                        email: "foo@bar.com",
+                        extraInfo: ["extra_key": "extra_value"]


Because extraInfo encoding depends on the feature (Logging and RUM supports multiple Encodable types, but Tracing encodes all values as String), it seems better to mix few types in this test:

extraInfo: ["extra_string": "string_value", "extra_int": 42, "extra_bool": true]

and document the expected encoding result in test assertions.

ncreated · 2020-11-26T09:56:38Z

Tests/DatadogTests/Datadog/TracerTests.swift

@@ -335,8 +335,8 @@ class TracerTests: XCTestCase {
        Datadog.setUserInfo(id: "abc-123", name: "Foo")
        tracer.startSpan(operationName: "span with user `id` and `name`").finish()

-        Datadog.setUserInfo(id: "abc-123", name: "Foo", email: "foo@example.com")
-        tracer.startSpan(operationName: "span with user `id`, `name` and `email`").finish()
+        Datadog.setUserInfo(id: "abc-123", name: "Foo", email: "foo@example.com", extraInfo: ["extra_key": "extra_value"])


Because extraInfo encoding depends on the feature (Logging and RUM supports multiple Encodable types, but Tracing encodes all values as String), it seems better to mix few types in this test:

extraInfo: ["extra_string": "string_value", "extra_int": 42, "extra_bool": true]

and document the expected encoding result in test assertions.

ncreated · 2020-11-26T09:57:04Z

Tests/DatadogTests/Datadog/LoggerTests.swift

@@ -186,16 +186,16 @@ class LoggerTests: XCTestCase {
        Datadog.setUserInfo(id: "abc-123", name: "Foo")
        logger.debug("message with user `id` and `name`")

-        Datadog.setUserInfo(id: "abc-123", name: "Foo", email: "foo@example.com")
-        logger.debug("message with user `id`, `name` and `email`")
+        Datadog.setUserInfo(id: "abc-123", name: "Foo", email: "foo@example.com", extraInfo: ["extra_key": "extra_value"])


Because extraInfo encoding depends on the feature (Logging and RUM supports multiple Encodable types, but Tracing encodes all values as String), it seems better to mix few types in this test:

extraInfo: ["extra_string": "string_value", "extra_int": 42, "extra_bool": true]

and document the expected encoding result in test assertions.

i can't pass different types in a dict to assertUserInfo method below
i will need to use matcher.assertValue(forKey:value:)

ncreated

All good 🚀

buranmert added this to the next-version milestone Nov 20, 2020

buranmert requested a review from ncreated November 20, 2020 17:27

buranmert self-assigned this Nov 20, 2020

buranmert force-pushed the buranmert/RUMM-886-user-extraInfo branch from ce3ada5 to 78cff21 Compare November 25, 2020 10:59

RUMM-886 User.ExtraInfo added

61246f6

buranmert force-pushed the buranmert/RUMM-886-user-extraInfo branch from 78cff21 to 61246f6 Compare November 25, 2020 18:34

buranmert marked this pull request as ready for review November 25, 2020 18:35

buranmert requested a review from a team as a code owner November 25, 2020 18:35

ncreated requested changes Nov 26, 2020

View reviewed changes

buranmert force-pushed the buranmert/RUMM-886-user-extraInfo branch from 482a35c to daa2209 Compare November 26, 2020 14:32

RUMM-886 PR comments addressed

72a19f2

buranmert force-pushed the buranmert/RUMM-886-user-extraInfo branch from daa2209 to 72a19f2 Compare November 26, 2020 14:50

buranmert requested a review from ncreated November 26, 2020 15:18

ncreated approved these changes Nov 26, 2020

View reviewed changes

buranmert merged commit 50e3d79 into master Nov 26, 2020

buranmert deleted the buranmert/RUMM-886-user-extraInfo branch November 26, 2020 16:10

ncreated removed this from the next-version milestone Dec 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUMM-886 `setUserInfo(..., extraInfo)` added #315

RUMM-886 `setUserInfo(..., extraInfo)` added #315

buranmert commented Nov 20, 2020 •

edited

ncreated commented Nov 24, 2020

ncreated left a comment

ncreated Nov 26, 2020

ncreated Nov 26, 2020

ncreated Nov 26, 2020

ncreated Nov 26, 2020

ncreated Nov 26, 2020

buranmert Nov 26, 2020

ncreated left a comment

		@@ -78,9 +78,15 @@ public class Datadog {
		public static func setUserInfo(

RUMM-886 setUserInfo(..., extraInfo) added #315

RUMM-886 setUserInfo(..., extraInfo) added #315

Conversation

buranmert commented Nov 20, 2020 • edited

What and why?

How?

Logs:

APM:

RUM:

Discussion 💭

Review checklist

ncreated commented Nov 24, 2020

ncreated left a comment

Choose a reason for hiding this comment

ncreated Nov 26, 2020

Choose a reason for hiding this comment

ncreated Nov 26, 2020

Choose a reason for hiding this comment

ncreated Nov 26, 2020

Choose a reason for hiding this comment

ncreated Nov 26, 2020

Choose a reason for hiding this comment

ncreated Nov 26, 2020

Choose a reason for hiding this comment

buranmert Nov 26, 2020

Choose a reason for hiding this comment

ncreated left a comment

Choose a reason for hiding this comment

RUMM-886 `setUserInfo(..., extraInfo)` added #315

RUMM-886 `setUserInfo(..., extraInfo)` added #315

buranmert commented Nov 20, 2020 •

edited