RUMM-1490 Migrate to Intake API v2 #562

ncreated · 2021-08-12T11:55:47Z

What and why?

🚚 This PR updates the SDK's upload layer for Intake API v2, by:

changing the path from /v1/input/<token> to /api/v2/<track-type> where track-type is rum, logs or spans;
adding new HTTP headers:
- DD-API-KEY - the client token,
- DD-EVP-ORIGIN - the origin sdk tag (e.g. ios or react-native),
- DD-EVP-ORIGIN-VERSION: the SDK version,
- DD-REQUEST-ID: a debug Intake request ID;
adding support to v2 HTTP response codes: 202, 400, 401, 403, 408, 413, 429, 500 and 503.

It also adds more verbosity to the console logs produced by DataUploadWorker, examples:

[DATADOG SDK] 🐶 → 10:13:05.933 [DEBUG]    → (rum) accepted, won't be retransmitted: [response code: 202 (accepted), request ID: 74293D75-FFEE-43A9-B328-70A71966AA69]
[DATADOG SDK] 🐶 → 10:15:17.138 [DEBUG]    → (logging) not delivered, will be retransmitted: [error: The Internet connection appears to be offline.]
[DATADOG SDK] 🐶 → 10:17:02.941 [DEBUG]    → (logging) accepted, won't be retransmitted: [response code: 403 (forbidden), request ID: 4DC9965B-357E-426D-B21C-4B61450C7634]

Last, it adds few improvements to the internal monitoring we do around the upload stack:

certain NSURLError codes are now ignored to not send false positives (e.g. when device goes offline it's not an error);
it handles new HTTP response codes to send errors on certain ones (e.g. 413 Payload Too Large).

How?

The upload architecture remains the same, but I made few changes to make testing easier with new HTTP status codes:

The "upload needs retry" consideration was moved from DataUploadWorker to the DataUploadStatus type. With that change, I was able to write simple tests for DataUploadStatus and only test two scenarios in DataUploadWorker: needs retry or does not.
As console log output highly depends on the HTTP status code (or network error), it is now part of the DataUploadStatus, which enables the same easy testing as for .needsRetry.

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 · 2021-08-12T12:36:52Z

Sources/Datadog/Core/Upload/DataUploadStatus.swift

+private let ignoredNSURLErrorCodes = Set([
+    NSURLErrorNetworkConnectionLost, // -1005
+    NSURLErrorTimedOut, // -1001
+    NSURLErrorCannotParseResponse, // - 1017
+    NSURLErrorNotConnectedToInternet, // -1009
+    NSURLErrorCannotFindHost, // -1003
+    NSURLErrorSecureConnectionFailed, // -1200
+    NSURLErrorDataNotAllowed, // -1020
+    NSURLErrorCannotConnectToHost, // -1004
+])


I made this list by looking at Datadog iOS app telemetry. None of these codes looks like SDK fault:

ncreated · 2021-08-12T12:41:53Z

Sources/Datadog/Core/Upload/DataUploadStatus.swift

+    case .badRequest, .payloadTooLarge, .requestTimeout:
+        // These codes mean that something wrong is happening either in the SDK or on the server - produce an error.
+        return (
+            message: "Data upload finished with status code: \(statusCode)",
+            error: nil,
+            attributes: ["dd_request_id": requestID ?? "(???)"]
+        )


Here, I'm not sure if 408 Request Timeout is worth sending error for Internal Monitoring. On one side this could indicate a slow response from Intake (monitored by other means), but on the other the SDK might be lagging on uploads. WDYT?

normally this case should be handled by URLSession, right? if the connection stays idle, it produces connection timed out error.
so i didn't fully understand what 408 means but i guess we can keep internal-monitor it anyway, if we can filter those errors out in Datadog UI.

maybe we can internal-monitor tooManyRequests too. as this shouldn't normally happen as we batch the requests, it can indicate a configuration problem in the client's side. wdyt?

we can merge once this discussion is resolved

There's distinction between client (URLSession) timeout and server timeout - 429 means the server.

maybe we can internal-monitor tooManyRequests too.

Good idea, I added .tooManyRequests to the list for alerting 👌.

ncreated · 2021-08-12T12:46:13Z

Sources/Datadog/Logging/LoggingFeature.swift

+                    .ddAPIKeyHeader(clientToken: configuration.clientToken),
+                    .ddEVPOriginHeader(source: configuration.common.source),
+                    .ddEVPOriginVersionHeader(),
+                    .ddRequestIDHeader(),


There is an open question from @0xnm (in DataDog/dd-sdk-android#671) if the DD-REQUEST-ID should be included all the time or only conditionally with a user setting.

buranmert

i left a few comments, not blocking but i wanted to discuss before approving

buranmert · 2021-08-16T07:28:23Z

Sources/Datadog/Core/Upload/DataUploadStatus.swift

+    case .badRequest, .payloadTooLarge, .requestTimeout:
+        // These codes mean that something wrong is happening either in the SDK or on the server - produce an error.
+        return (
+            message: "Data upload finished with status code: \(statusCode)",
+            error: nil,
+            attributes: ["dd_request_id": requestID ?? "(???)"]
+        )


normally this case should be handled by URLSession, right? if the connection stays idle, it produces connection timed out error.
so i didn't fully understand what 408 means but i guess we can keep internal-monitor it anyway, if we can filter those errors out in Datadog UI.

maybe we can internal-monitor tooManyRequests too. as this shouldn't normally happen as we batch the requests, it can indicate a configuration problem in the client's side. wdyt?

buranmert · 2021-08-16T07:52:47Z

Sources/Datadog/Core/Upload/DataUploader.swift

            }

            semaphore.signal()
        }

        _ = semaphore.wait(timeout: .distantFuture)

-        return uploadStatus ?? .unknown
+        return uploadStatus ?? DataUploader.unreachableUploadStatus


maybe we can see this case as completion not called error or something along those lines. DataUploader.unreachableUploadStatus isn't very intiutive at the first sight.

example, feel free to change the details:

private static let nonCalledCompletionStatus = DataUploadStatus(needsRetry: false, userDebugDescription: "An internal error occurred", userErrorMessage: nil, internalMonitoringError: "HTTPClient Completion not called")

i mean, at least the status value should try to explain the situation

The ?? DataUploader.unreachableUploadStatus here is to only satisfy the compiler - the right side of ?? operator will never be evaluated as enforced by the previous logic in this function. That's why the name and comment:

/// An unreachable upload status - only meant to satisfy the compiler. private static let unreachableUploadStatus = DataUploadStatus(...)

Alternative to this would be:

guard let uploadStatus = uploadStatus else { fatalError() } return uploadStatus

but as we avoid using fatalError() in the SDK codebase, I adopted the solution which is already used in other places: returning known default as fallback.

Hence I don't think we should improve the error value here - this would give a false sense of reachability / usability.

Sources/Datadog/Core/Feature.swift

buranmert · 2021-08-16T08:37:28Z

Sources/Datadog/Core/Upload/HTTPHeadersProvider.swift

+        // MARK: - Datadog Headers
+
+        /// Datadog request authentication header.
+        static func ddAPIKeyHeader(clientToken: String) -> HTTPHeader {


this might be confusing for the outsiders/newcomers as we have client tokens and API keys

what do you think of renaming this function to ddClientTokenHeader? (of course we will keep "DD-API-KEY" as value)

Here I used consistent convention for 3 new headers:

static func ddAPIKeyHeader(clientToken: String) -> HTTPHeader static func ddEVPOriginHeader(source: String) -> HTTPHeader static func ddEVPOriginVersionHeader() -> HTTPHeader

to make it clearly visible that we follow the spec for all features:

// in LoggingFeature, TracingFeature, RUMFeature and InternalMonitoringFeature: requestBuilder: RequestBuilder( // ... headers: [ // ... .ddAPIKeyHeader(clientToken: configuration.clientToken), .ddEVPOriginHeader(source: configuration.common.source), .ddEVPOriginVersionHeader(), ] )

The spec says we must send DD-API-KEY, DD-EVP-ORIGIN and DD-EVP-ORIGIN-VERSION headers - and I shaped the above code to tell exactly that story.

IMO if we reverse it by naming the function ddClientTokenHeader(), the DD-API-KEY value used internally would be equally puzzling to the reader.

…tructs into `RequestBuilder`

xgouchet

LGTM

…Y REQUESTS status code

ncreated self-assigned this Aug 12, 2021

ncreated commented Aug 12, 2021

View reviewed changes

ncreated marked this pull request as ready for review August 12, 2021 12:55

ncreated requested a review from a team as a code owner August 12, 2021 12:55

ncreated added 6 commits August 12, 2021 18:49

RUMM-1490 Update upload URL to API v2 for all features

bf5e3ed

RUMM-1490 Configure API v2 HTTP headers for all features

5b1d8d6

RUMM-1490 Rename UploadURLProvider to UploadURL

dc5b4a3

RUMM-1490 Handle all supported response codes in API v2

c5329ce

RUMM-1490 Update integration tests to match API v2 requests

5f891f9

RUMM-1490 Cleanup

c4cdb66

ncreated force-pushed the ncreated/RUMM-1490-migrate-to-intake-api-v2 branch from f6491ec to c4cdb66 Compare August 12, 2021 16:49

buranmert reviewed Aug 16, 2021

View reviewed changes

RUMM-1490 CR feedback - merge HTTP Headers provider and Data URL cons…

3b0f690

…tructs into `RequestBuilder`

xgouchet approved these changes Aug 17, 2021

View reviewed changes

ncreated added 2 commits August 17, 2021 10:40

RUMM-1490 CR fix - print client token warning on 401 not 403

48b48a4

RUMM-1490 CR feedback - send internal monitoring error on 429 TOO MAN…

94a2234

…Y REQUESTS status code

buranmert approved these changes Aug 17, 2021

View reviewed changes

ncreated merged commit c1b594e into master Aug 23, 2021

ncreated deleted the ncreated/RUMM-1490-migrate-to-intake-api-v2 branch August 23, 2021 09:39

ncreated mentioned this pull request Oct 11, 2021

RUMM-1679 Compress HTTP body using deflate (ETF RFC 1950) #626

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUMM-1490 Migrate to Intake API v2 #562

RUMM-1490 Migrate to Intake API v2 #562

ncreated commented Aug 12, 2021 •

edited

Loading

ncreated Aug 12, 2021

ncreated Aug 12, 2021

buranmert Aug 16, 2021

buranmert Aug 17, 2021

ncreated Aug 17, 2021

ncreated Aug 12, 2021

buranmert left a comment

buranmert Aug 16, 2021

buranmert Aug 16, 2021

ncreated Aug 16, 2021

buranmert Aug 16, 2021

ncreated Aug 16, 2021

xgouchet left a comment

RUMM-1490 Migrate to Intake API v2 #562

RUMM-1490 Migrate to Intake API v2 #562

Conversation

ncreated commented Aug 12, 2021 • edited Loading

What and why?

How?

Review checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buranmert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xgouchet left a comment

Choose a reason for hiding this comment

ncreated commented Aug 12, 2021 •

edited

Loading