Skip to content

Commit

Permalink
RUMM-1490 Cleanup
Browse files Browse the repository at this point in the history
  • Loading branch information
ncreated committed Aug 12, 2021
1 parent 40bf518 commit f6491ec
Show file tree
Hide file tree
Showing 11 changed files with 67 additions and 127 deletions.
2 changes: 1 addition & 1 deletion Sources/Datadog/Core/Feature.swift
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ internal struct FeatureUpload {
let dataUploader = DataUploader(
httpClient: commonDependencies.httpClient,
uploadURL: uploadURL,
httpHeadersProvider: uploadHTTPHeadersProvider
headersProvider: uploadHTTPHeadersProvider
)

self.init(
Expand Down
8 changes: 2 additions & 6 deletions Sources/Datadog/Core/Upload/DataUploadWorker.swift
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,8 @@ internal class DataUploadWorker: DataUploadWorkerType {
}

// Send internal monitoring error (if any and if Internal Monitoring is enabled)
if let internalMonitoringError = uploadStatus.internalMonitoringError {
self.internalMonitor?.sdkLogger.error(
internalMonitoringError.message,
error: internalMonitoringError.error,
attributes: internalMonitoringError.attributes
)
if let sdkError = uploadStatus.internalMonitoringError {
self.internalMonitor?.sdkLogger.error(sdkError.message, error: sdkError.error, attributes: sdkError.attributes)
}
} else {
let batchLabel = nextBatch != nil ? "YES" : (isSystemReady ? "NO" : "NOT CHECKED")
Expand Down
21 changes: 6 additions & 15 deletions Sources/Datadog/Core/Upload/DataUploader.swift
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,6 @@ internal class UploadURL {
urlComponents?.queryItems = queryItems.map { $0.urlQueryItem }
}

if urlComponents?.url == nil { // sanity check - should not happen
userLogger.error("🔥 Failed to create upload URL from \(url) and \(queryItems)")
}

self.url = urlComponents?.url ?? url
}
}
Expand All @@ -48,26 +44,21 @@ internal protocol DataUploaderType {

/// Synchronously uploads data to server using `HTTPClient`.
internal final class DataUploader: DataUploaderType {
/// An unreachable upload status - mean to only satisfy compiler.
private static let unreachableUploadStatus = DataUploadStatus(
needsRetry: false,
userDebugDescription: "",
userErrorMessage: nil,
internalMonitoringError: nil
)
/// An unreachable upload status - only meant to satisfy the compiler.
private static let unreachableUploadStatus = DataUploadStatus(needsRetry: false, userDebugDescription: "", userErrorMessage: nil, internalMonitoringError: nil)

private let httpClient: HTTPClient
private let uploadURL: UploadURL
private let httpHeadersProvider: HTTPHeadersProvider
private let headersProvider: HTTPHeadersProvider

init(
httpClient: HTTPClient,
uploadURL: UploadURL,
httpHeadersProvider: HTTPHeadersProvider
headersProvider: HTTPHeadersProvider
) {
self.httpClient = httpClient
self.uploadURL = uploadURL
self.httpHeadersProvider = httpHeadersProvider
self.headersProvider = headersProvider
}

/// Uploads data synchronously (will block current thread) and returns the upload status.
Expand Down Expand Up @@ -96,7 +87,7 @@ internal final class DataUploader: DataUploaderType {

private func createRequestWith(data: Data) -> (request: URLRequest, ddRequestID: String?) {
var request = URLRequest(url: uploadURL.url)
let headers = httpHeadersProvider.headers
let headers = headersProvider.headers
request.httpMethod = "POST"
request.allHTTPHeaderFields = headers
request.httpBody = data
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class DataUploaderBenchmarkTests: BenchmarkTests {
let dataUploader = DataUploader(
httpClient: HTTPClient(),
uploadURL: mockUploadURL(),
httpHeadersProvider: HTTPHeadersProvider(headers: [])
headersProvider: HTTPHeadersProvider(headers: [])
)

// `measure` runs 5 iterations
Expand Down
21 changes: 14 additions & 7 deletions Tests/DatadogTests/Datadog/Core/FeaturesConfigurationTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ class FeaturesConfigurationTests: XCTestCase {
XCTAssertEqual(configuration.logging?.clientToken, clientToken)
XCTAssertEqual(configuration.tracing?.clientToken, clientToken)
XCTAssertEqual(configuration.rum?.clientToken, clientToken)
XCTAssertNotEqual(configuration.internalMonitoring?.clientToken, clientToken)
}

func testEndpoint() throws {
Expand Down Expand Up @@ -579,13 +580,16 @@ class FeaturesConfigurationTests: XCTestCase {
func testWhenInternalMonitoringClientTokenIsSet_thenInternalMonitoringConfigurationIsEnabled() throws {
// When
let internalMonitoringClientToken: String = .mockRandom(among: "abcdef")
let featuresClientToken: String = .mockRandom(among: "ghijkl")
let featuresConfiguration = try FeaturesConfiguration(
configuration: .mockWith(internalMonitoringClientToken: internalMonitoringClientToken),
appContext: .mockWith(
bundleIdentifier: "com.bundle.identifier",
bundleVersion: "1.2.3",
bundleName: "AppName"
)
configuration: .mockWith(
clientToken: featuresClientToken,
loggingEnabled: true,
tracingEnabled: true,
rumEnabled: true,
internalMonitoringClientToken: internalMonitoringClientToken
),
appContext: .mockAny()
)

// Then
Expand All @@ -597,7 +601,10 @@ class FeaturesConfigurationTests: XCTestCase {
configuration.logsUploadURL.absoluteString,
"https://logs.browser-intake-datadoghq.com/api/v2/logs"
)
XCTAssertEqual(configuration.clientToken, internalMonitoringClientToken)
XCTAssertEqual(configuration.clientToken, internalMonitoringClientToken, "Internal Monitoring must use monitoring token")
XCTAssertEqual(featuresConfiguration.logging!.clientToken, featuresClientToken, "Logging must use feature token")
XCTAssertEqual(featuresConfiguration.tracing!.clientToken, featuresClientToken, "Tracing must use feature token")
XCTAssertEqual(featuresConfiguration.rum!.clientToken, featuresClientToken, "RUM must use feature token")
}

// MARK: - Invalid Configurations
Expand Down
118 changes: 34 additions & 84 deletions Tests/DatadogTests/Datadog/Core/Upload/DataUploadStatusTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -29,21 +29,15 @@ class DataUploadStatusTests: XCTestCase {

func testWhenUploadFinishesWithResponse_andStatusCodeNeedsNoRetry_itSetsNeedsRetryFlagToFalse() {
statusCodesExpectingNoRetry.forEach { statusCode in
let uploadStatus = DataUploadStatus(
httpResponse: .mockResponseWith(statusCode: statusCode),
ddRequestID: .mockAny()
)
XCTAssertFalse(uploadStatus.needsRetry, "Upload should not be retried for status code \(statusCode)")
let status = DataUploadStatus(httpResponse: .mockResponseWith(statusCode: statusCode), ddRequestID: .mockAny())
XCTAssertFalse(status.needsRetry, "Upload should not be retried for status code \(statusCode)")
}
}

func testWhenUploadFinishesWithResponse_andStatusCodeNeedsRetry_itSetsNeedsRetryFlagToTrue() {
statusCodesExpectingRetry.forEach { statusCode in
let uploadStatus = DataUploadStatus(
httpResponse: .mockResponseWith(statusCode: statusCode),
ddRequestID: .mockAny()
)
XCTAssertTrue(uploadStatus.needsRetry, "Upload should be retried for status code \(statusCode)")
let status = DataUploadStatus(httpResponse: .mockResponseWith(statusCode: statusCode), ddRequestID: .mockAny())
XCTAssertTrue(status.needsRetry, "Upload should be retried for status code \(statusCode)")
}
}

Expand All @@ -52,93 +46,67 @@ class DataUploadStatusTests: XCTestCase {
let unexpectedStatusCodes = allStatusCodes.subtracting(Set(expectedStatusCodes))

unexpectedStatusCodes.forEach { statusCode in
let uploadStatus = DataUploadStatus(
httpResponse: .mockResponseWith(statusCode: statusCode),
ddRequestID: .mockAny()
)
XCTAssertFalse(uploadStatus.needsRetry, "Upload should not be retried for status code \(statusCode)")
let status = DataUploadStatus(httpResponse: .mockResponseWith(statusCode: statusCode), ddRequestID: .mockAny())
XCTAssertFalse(status.needsRetry, "Upload should not be retried for status code \(statusCode)")
}
}

func testWhenUploadFinishesWithError_itSetsNeedsRetryFlagToTrue() {
let uploadStatus = DataUploadStatus(networkError: ErrorMock())
XCTAssertTrue(uploadStatus.needsRetry, "Upload should be retried if it finished with error")
let status = DataUploadStatus(networkError: ErrorMock())
XCTAssertTrue(status.needsRetry, "Upload should be retried if it finished with error")
}

// MARK: - Test `.userDebugDescription`

func testWhenUploadFinishesWithResponse_andRequestIDIsAvailable_itCreatesUserDebugDescription() {
expectedStatusCodes.forEach { statusCode in
let requestID: String = .mockRandom(among: .alphanumerics)

let uploadStatus = DataUploadStatus(
httpResponse: .mockResponseWith(statusCode: statusCode),
ddRequestID: requestID
)

let status = DataUploadStatus(httpResponse: .mockResponseWith(statusCode: statusCode), ddRequestID: requestID)
XCTAssertTrue(
uploadStatus.userDebugDescription.matches(
status.userDebugDescription.matches(
regex: "\\[response code: [0-9]{3} \\([a-zA-Z]+\\), request ID: \(requestID)\\]"
),
"'\(uploadStatus.userDebugDescription)' is not expected description for status code '\(statusCode)' and request id '\(requestID)'"
"'\(status.userDebugDescription)' is not an expected description for status code '\(statusCode)' and request id '\(requestID)'"
)
}
}

func testWhenUploadFinishesWithResponse_andRequestIDIsNotAvailable_itCreatesUserDebugDescription() {
expectedStatusCodes.forEach { statusCode in
let uploadStatus = DataUploadStatus(
httpResponse: .mockResponseWith(statusCode: statusCode),
ddRequestID: nil
)

let status = DataUploadStatus(httpResponse: .mockResponseWith(statusCode: statusCode), ddRequestID: nil)
XCTAssertTrue(
uploadStatus.userDebugDescription.matches(
status.userDebugDescription.matches(
regex: "\\[response code: [0-9]{3} \\([a-zA-Z]+\\), request ID: \\(\\?\\?\\?\\)\\]"
),
"'\(uploadStatus.userDebugDescription)' is not expected description for status code '\(statusCode)' and no request id"
"'\(status.userDebugDescription)' is not an expected description for status code '\(statusCode)' and no request id"
)
}
}

func testWhenUploadFinishesWithError_itCreatesUserDebugDescription() {
let randomErrorDescription: String = .mockRandom()

let uploadStatus = DataUploadStatus(
networkError: ErrorMock(randomErrorDescription)
)

XCTAssertEqual(uploadStatus.userDebugDescription, "[error: \(randomErrorDescription)]")
let status = DataUploadStatus(networkError: ErrorMock(randomErrorDescription))
XCTAssertEqual(status.userDebugDescription, "[error: \(randomErrorDescription)]")
}

// MARK: - Test `.userErrorMessage`

func testWhenUploadFinishesWithResponse_andStatusCodeIs403_itCreatesClientTokenErrorMessage() {
let uploadStatus = DataUploadStatus(
httpResponse: .mockResponseWith(statusCode: 403),
ddRequestID: nil
)

XCTAssertEqual(uploadStatus.userErrorMessage, "⚠️ The client token you provided seems to be invalid.")
let status = DataUploadStatus(httpResponse: .mockResponseWith(statusCode: 403), ddRequestID: nil)
XCTAssertEqual(status.userErrorMessage, "⚠️ The client token you provided seems to be invalid.")
}

func testWhenUploadFinishesWithResponse_andStatusCodeIsDifferentThan401_itDoesNotCreateAnyUserErrorMessage() {
var statusCodes = Set((100...599))
statusCodes.remove(403)

func testWhenUploadFinishesWithResponse_andStatusCodeIsDifferentThan403_itDoesNotCreateAnyUserErrorMessage() {
let statusCodes = Set((100...599)).subtracting([403])
statusCodes.forEach { statusCode in
let uploadStatus = DataUploadStatus(
httpResponse: .mockResponseWith(statusCode: statusCode),
ddRequestID: nil
)

XCTAssertNil(uploadStatus.userErrorMessage)
let status = DataUploadStatus(httpResponse: .mockResponseWith(statusCode: statusCode), ddRequestID: nil)
XCTAssertNil(status.userErrorMessage)
}
}

func testWhenUploadFinishesWithError_itDoesNotCreateAnyUserErrorMessage() {
let uploadStatus = DataUploadStatus(networkError: ErrorMock(.mockRandom()))
XCTAssertNil(uploadStatus.userErrorMessage)
let status = DataUploadStatus(networkError: ErrorMock(.mockRandom()))
XCTAssertNil(status.userErrorMessage)
}

// MARK: - Test `.internalMonitoringError`
Expand All @@ -152,60 +120,42 @@ class DataUploadStatusTests: XCTestCase {
func testWhenUploadFinishesWithResponse_andStatusCodeMeansSDKIssue_itCreatesInternalMonitoringError() throws {
try alertingStatusCodes.forEach { statusCode in
let requestID: String = .mockRandom()

let uploadStatus = DataUploadStatus(
httpResponse: .mockResponseWith(statusCode: statusCode),
ddRequestID: requestID
)

let error = try XCTUnwrap(uploadStatus.internalMonitoringError, "Internal Monitoring error should be created for status code \(statusCode)")
let status = DataUploadStatus(httpResponse: .mockResponseWith(statusCode: statusCode), ddRequestID: requestID)
let error = try XCTUnwrap(status.internalMonitoringError, "Internal Monitoring error should be created for status code \(statusCode)")
XCTAssertEqual(error.message, "Data upload finished with status code: \(statusCode)")
XCTAssertEqual(error.attributes?["dd_request_id"], requestID)
}
}

func testWhenUploadFinishesWithResponse_andStatusCodeMeansClientIssue_itDoesNotCreateInternalMonitoringError() {
let clientIssueStatusCodes = Set(expectedStatusCodes).subtracting(Set(alertingStatusCodes))

clientIssueStatusCodes.forEach { statusCode in
let uploadStatus = DataUploadStatus(
httpResponse: .mockResponseWith(statusCode: statusCode),
ddRequestID: nil
)

XCTAssertNil(
uploadStatus.internalMonitoringError,
"Internal Monitoring error should not be created for status code \(statusCode)"
)
let status = DataUploadStatus(httpResponse: .mockResponseWith(statusCode: statusCode), ddRequestID: nil)
XCTAssertNil(status.internalMonitoringError, "Internal Monitoring error should not be created for status code \(statusCode)")
}
}

func testWhenUploadFinishesWithResponse_andUnexpectedStatusCodeMeansClientIssue_itDoesNotCreateInternalMonitoringError() {
let unexpectedStatusCodes = Set((100...599)).subtracting(Set(expectedStatusCodes))

unexpectedStatusCodes.forEach { statusCode in
let uploadStatus = DataUploadStatus(
httpResponse: .mockResponseWith(statusCode: statusCode),
ddRequestID: nil
)

XCTAssertNil(uploadStatus.internalMonitoringError)
let status = DataUploadStatus(httpResponse: .mockResponseWith(statusCode: statusCode), ddRequestID: nil)
XCTAssertNil(status.internalMonitoringError)
}
}

func testWhenUploadFinishesWithError_andErrorCodeMeansSDKIssue_itCreatesInternalMonitoringError() throws {
let alertingNSURLErrorCode = NSURLErrorBadURL
let uploadStatus = DataUploadStatus(networkError: NSError(domain: NSURLErrorDomain, code: alertingNSURLErrorCode, userInfo: nil))
let status = DataUploadStatus(networkError: NSError(domain: NSURLErrorDomain, code: alertingNSURLErrorCode, userInfo: nil))

let error = try XCTUnwrap(uploadStatus.internalMonitoringError, "Internal Monitoring error should be created for NSURLError code: \(alertingNSURLErrorCode)")
let error = try XCTUnwrap(status.internalMonitoringError, "Internal Monitoring error should be created for NSURLError code: \(alertingNSURLErrorCode)")
XCTAssertEqual(error.message, "Data upload finished with error")
let nsError = try XCTUnwrap(error.error) as NSError
XCTAssertEqual(nsError.code, alertingNSURLErrorCode)
}

func testWhenUploadFinishesWithError_andErrorCodeMeansExternalFactors_itDoesNotCreateInternalMonitoringError() {
let notAlertingNSURLErrorCode = NSURLErrorNetworkConnectionLost
let uploadStatus = DataUploadStatus(networkError: NSError(domain: NSURLErrorDomain, code: notAlertingNSURLErrorCode, userInfo: nil))
XCTAssertNil(uploadStatus.internalMonitoringError, "Internal Monitoring error should be created for NSURLError code: \(notAlertingNSURLErrorCode)")
let status = DataUploadStatus(networkError: NSError(domain: NSURLErrorDomain, code: notAlertingNSURLErrorCode, userInfo: nil))
XCTAssertNil(status.internalMonitoringError, "Internal Monitoring error should be created for NSURLError code: \(notAlertingNSURLErrorCode)")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class DataUploadWorkerTests: XCTestCase {
let dataUploader = DataUploader(
httpClient: HTTPClient(session: server.getInterceptedURLSession()),
uploadURL: .mockAny(),
httpHeadersProvider: .mockAny()
headersProvider: .mockAny()
)

// Given
Expand Down Expand Up @@ -144,7 +144,7 @@ class DataUploadWorkerTests: XCTestCase {
let dataUploader = DataUploader(
httpClient: HTTPClient(session: server.getInterceptedURLSession()),
uploadURL: .mockAny(),
httpHeadersProvider: .mockAny()
headersProvider: .mockAny()
)
let worker = DataUploadWorker(
queue: uploaderQueue,
Expand Down Expand Up @@ -178,7 +178,7 @@ class DataUploadWorkerTests: XCTestCase {
let dataUploader = DataUploader(
httpClient: HTTPClient(session: server.getInterceptedURLSession()),
uploadURL: .mockAny(),
httpHeadersProvider: .mockAny()
headersProvider: .mockAny()
)
let worker = DataUploadWorker(
queue: uploaderQueue,
Expand Down Expand Up @@ -212,7 +212,7 @@ class DataUploadWorkerTests: XCTestCase {
let dataUploader = DataUploader(
httpClient: HTTPClient(session: server.getInterceptedURLSession()),
uploadURL: .mockAny(),
httpHeadersProvider: .mockAny()
headersProvider: .mockAny()
)
let worker = DataUploadWorker(
queue: uploaderQueue,
Expand Down Expand Up @@ -301,7 +301,7 @@ class DataUploadWorkerTests: XCTestCase {
let dataUploader = DataUploader(
httpClient: HTTPClient(session: server.getInterceptedURLSession()),
uploadURL: .mockAny(),
httpHeadersProvider: .mockAny()
headersProvider: .mockAny()
)
let worker = DataUploadWorker(
queue: uploaderQueue,
Expand All @@ -326,7 +326,7 @@ class DataUploadWorkerTests: XCTestCase {
let dataUploader = DataUploader(
httpClient: HTTPClient(session: server.getInterceptedURLSession()),
uploadURL: .mockAny(),
httpHeadersProvider: .mockAny()
headersProvider: .mockAny()
)
let worker = DataUploadWorker(
queue: uploaderQueue,
Expand Down
Loading

0 comments on commit f6491ec

Please sign in to comment.