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

RUMM-182 Sync logs time with server time #65

Merged
merged 1 commit into from
Apr 9, 2020

Conversation

buranmert
Copy link
Contributor

@buranmert buranmert commented Apr 2, 2020

What and why?

• SDK needs to send current date in miliseconds in order for the backend to be able to tell time difference between the device and actual time.
• This is especially essential for tracing among different systems.

How?

  1. DataUploadURL can change query of its URL dynamically
  2. URLQueryProvider is made enum so that we limit the number of cases to test

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

@buranmert buranmert requested a review from ncreated April 2, 2020 13:23
@buranmert buranmert self-assigned this Apr 2, 2020
@buranmert buranmert force-pushed the buranmert/RUMM-182-sync-time branch from 5c695d1 to 3fad6df Compare April 2, 2020 15:44
@buranmert buranmert force-pushed the buranmert/RUMM-182-sync-time branch from 3fad6df to a97af1c Compare April 3, 2020 11:27
@buranmert buranmert marked this pull request as ready for review April 3, 2020 11:27
@buranmert buranmert requested a review from a team as a code owner April 3, 2020 11:27
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it 🏅. Let me know WDYT about my ideas 🙂.

@buranmert buranmert force-pushed the buranmert/RUMM-182-sync-time branch from a97af1c to 171e5dc Compare April 6, 2020 15:05
@buranmert buranmert added this to the next-version milestone Apr 7, 2020
}
}

/// Synchronously uploads data to server using `HTTPClient`.
internal final class DataUploader {
private let uploadURL: DataUploadURL
private let uploadURL: UploadURLProvider
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is a bit confusing to keep naming it uploadUrl , why not uploadUrlProvider ?

@@ -103,14 +103,14 @@ public class Datadog {

internal convenience init(
appContext: AppContext,
logsUploadURL: DataUploadURL,
logsUploadURL: UploadURLProvider,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logsUploadUrlProvider ?

@buranmert buranmert force-pushed the buranmert/RUMM-182-sync-time branch from 171e5dc to 3c756b7 Compare April 8, 2020 13:48
Comment on lines -28 to -49
func testUSLogsEndpoint() {
XCTAssertEqual(
Configuration.builderUsing(clientToken: "abcd").set(logsEndpoint: .us).build().logsUploadURL?.url,
URL(string: "https://mobile-http-intake.logs.datadoghq.com/v1/input/abcd?ddsource=mobile")!
)
}

func testEULogsEndpoint() {
XCTAssertEqual(
Configuration.builderUsing(clientToken: "abcd").set(logsEndpoint: .eu).build().logsUploadURL?.url,
URL(string: "https://mobile-http-intake.logs.datadoghq.eu/v1/input/abcd?ddsource=mobile")!
)
}

func testCustomLogsEndpoint() {
XCTAssertEqual(
Configuration.builderUsing(clientToken: "abcd")
.set(logsEndpoint: .custom(url: "https://api.example.com/v1/logs/"))
.build().logsUploadURL?.url,
URL(string: "https://api.example.com/v1/logs/abcd?ddsource=mobile")!
)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I think these tests should remain there, as we lost coverage for se(logsEndpoint:) public API:

Screenshot 2020-04-08 at 16 58 24

@buranmert buranmert force-pushed the buranmert/RUMM-182-sync-time branch from 3c756b7 to b68cef8 Compare April 8, 2020 16:29

let dateAgo = date15Dec2019 - 0.001
XCTAssertEqual(dateAgo.currentTimeMillis, 1_576_403_999_999)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also include test for the code branch which captures overflow, this can be easily done by using "reference date" as relative base:

Suggested change
}
}
func testUInt64Overflow() {
let dateWithOverflow = Date(timeIntervalSinceReferenceDate: .greatestFiniteMagnitude)
XCTAssertEqual(dateWithOverflow.currentTimeMillis, UInt64.max)
}

Copy link
Contributor Author

@buranmert buranmert Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea 💡
i just noticed i forgot to add tests for init(withReportingOverflow:) too

EDIT done ✅

class DatadogConfigurationTests: XCTestCase {
private typealias Configuration = Datadog.Configuration

func testDefaultConfiguration() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer test Configuration.Builder behaviours. In fact, when we switch default endpoint from .us to .eu, none of Swift unit tests do fail.

This will test builder logic:

import XCTest
@testable import Datadog

class DatadogConfigurationTests: XCTestCase {
    private typealias Configuration = Datadog.Configuration

    func testDefaultConfiguration() {
        let defaultConfiguration = Configuration.builderUsing(clientToken: "abcd").build()
        XCTAssertEqual(defaultConfiguration.clientToken, "abcd")
        XCTAssertEqual(defaultConfiguration.logsEndpoint.url, "https://mobile-http-intake.logs.datadoghq.com/v1/input/")
    }

    // MARK: - Logs endpoint
    func testUSLogsEndpoint() {
        let configuration = Configuration.builderUsing(clientToken: .mockAny()).set(logsEndpoint: .us).build()
        XCTAssertEqual(configuration.logsEndpoint.url, "https://mobile-http-intake.logs.datadoghq.com/v1/input/")
    }

    func testEULogsEndpoint() {
        let configuration = Configuration.builderUsing(clientToken: .mockAny()).set(logsEndpoint: .eu).build()
        XCTAssertEqual(configuration.logsEndpoint.url, "https://mobile-http-intake.logs.datadoghq.eu/v1/input/")
    }

    func testCustomLogsEndpoint() {
        let configuration = Configuration.builderUsing(clientToken: .mockAny())
            .set(logsEndpoint: .custom(url: "https://api.example.com/v1/logs/"))
            .build()
        XCTAssertEqual(configuration.logsEndpoint.url, "https://api.example.com/v1/logs/")
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copied&pasted ✅

Comment on lines 34 to 65

// MARK: - Logs endpoint

func testUSLogsEndpoint() {
let urlUS = Datadog.Configuration.LogsEndpoint.us.url
let urlProvider = try! UploadURLProvider(
endpointURL: urlUS,
clientToken: "abc",
dateProvider: dateProvider
)
XCTAssertEqual(urlProvider.url, URL(string: "https://mobile-http-intake.logs.datadoghq.com/v1/input/abc?ddsource=mobile&batch_time=1576404000000"))
}

func testEULogsEndpoint() {
let urlEU = Datadog.Configuration.LogsEndpoint.eu.url
let urlProvider = try! UploadURLProvider(
endpointURL: urlEU,
clientToken: "abc",
dateProvider: dateProvider
)
XCTAssertEqual(urlProvider.url, URL(string: "https://mobile-http-intake.logs.datadoghq.eu/v1/input/abc?ddsource=mobile&batch_time=1576404000000"))
}

func testCustomLogsEndpoint() {
let urlCustom = Datadog.Configuration.LogsEndpoint.custom(url: "foo.bar").url
let urlProvider = try! UploadURLProvider(
endpointURL: urlCustom,
clientToken: "abc",
dateProvider: dateProvider
)
XCTAssertEqual(urlProvider.url, URL(string: "foo.bar/abc?ddsource=mobile&batch_time=1576404000000"))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those tests do exactly the same assertion as func testItBuildsValidURL() - "When a given URL string is provided, UploadURLProvider decorates it with expected URL queries". Asserting values of the URL for different endpoints (.us, .eu, .custom(...)) is a better fit for configuration builder tests (DatadogConfigurationTests) as we also need to assert the defaults it produces (when no endpoint is set explicitly).

In other words:

  • in DataUploaderTests we can test:
    • .us endpoint string
    • .eu endpoint string
    • .custom endpoint string
  • in DatadogConfigurationTests we could test:
    • .us endpoint string
    • .eu endpoint string
    • .custom endpoint string
    • default endpoint if not explicitly set by the user 👈
Suggested change
// MARK: - Logs endpoint
func testUSLogsEndpoint() {
let urlUS = Datadog.Configuration.LogsEndpoint.us.url
let urlProvider = try! UploadURLProvider(
endpointURL: urlUS,
clientToken: "abc",
dateProvider: dateProvider
)
XCTAssertEqual(urlProvider.url, URL(string: "https://mobile-http-intake.logs.datadoghq.com/v1/input/abc?ddsource=mobile&batch_time=1576404000000"))
}
func testEULogsEndpoint() {
let urlEU = Datadog.Configuration.LogsEndpoint.eu.url
let urlProvider = try! UploadURLProvider(
endpointURL: urlEU,
clientToken: "abc",
dateProvider: dateProvider
)
XCTAssertEqual(urlProvider.url, URL(string: "https://mobile-http-intake.logs.datadoghq.eu/v1/input/abc?ddsource=mobile&batch_time=1576404000000"))
}
func testCustomLogsEndpoint() {
let urlCustom = Datadog.Configuration.LogsEndpoint.custom(url: "foo.bar").url
let urlProvider = try! UploadURLProvider(
endpointURL: urlCustom,
clientToken: "abc",
dateProvider: dateProvider
)
XCTAssertEqual(urlProvider.url, URL(string: "foo.bar/abc?ddsource=mobile&batch_time=1576404000000"))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done ✅

UploadURLProvider dynamically injects current date as query to URLs
@buranmert buranmert force-pushed the buranmert/RUMM-182-sync-time branch from b68cef8 to 717f567 Compare April 9, 2020 08:44
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💪

@buranmert buranmert merged commit 7d503d9 into master Apr 9, 2020
@buranmert buranmert deleted the buranmert/RUMM-182-sync-time branch April 9, 2020 09:24
@buranmert buranmert mentioned this pull request Apr 20, 2020
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants