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-437 Encoding doesn't ignore miliseconds anymore #96

Merged
merged 1 commit into from
May 5, 2020

Conversation

buranmert
Copy link
Contributor

@buranmert buranmert commented May 4, 2020

Closes #95

What and why?

SDK collects Logs and writes them onto disk, then uploads them as raw data
It relies on JSONEncoder with dateEncodingStrategy = .iso8601
However, it turns out IS08601 doesn't enforce miliseconds and JSONEncoder omits them

let date = {2020-05-04T18:19:57.109Z}
jsonEncoder.dateEncodingStrategy = .iso8601
jsonEncoder.encode(date) // becomes {2020-05-04T18:19:57Z}

The result is very well described in the original issue #95

How?

There are 2 main solutions:

ISO8601DateFormatter and dateEncodingStrategy = .custom

🍎 Apple recommends using ISO8601DateFormatter when dealing with ISO8601 format.
❌ However, that enforces us to use dateEncodingStrategy = .custom
Because the logical strategy option, .formatted(DateFormatter), doesn't accept it as it's not a subclass of DateFormatter

Customized DateFormatter and dateEncodingStrategy = .formatted

Although this is not the official way, this method is used in many places/projects.

Basically, we describe the ISO8601 standard in our own DateFormatter instance
Then we pass the instance to dateEncodingStrategy = .formatted(dateFormatter)
✅ Now we have miliseconds in the encoded data

UPDATE:

I turned to the first option: ISO8601DateFormatter and dateEncodingStrategy = .custom

Questions?

How can we test encoding without making types Decodable, since they don't need to be decoded in the main target?

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 added the bug Something isn't working label May 4, 2020
@buranmert buranmert added this to the next-version milestone May 4, 2020
@buranmert buranmert requested a review from a team as a code owner May 4, 2020 18:31
@buranmert buranmert self-assigned this May 4, 2020
Copy link
Collaborator

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

let date = {2020-05-04T18:19:57.109Z}
jsonEncoder.dateEncodingStrategy = .iso8601
jsonEncoder.encode(date) // becomes {2020-05-04T18:19:57Z}

That's super interesting 👌. Is this factorial precision lost somewhere documented by Apple?

How can we test encoding without making types Decodable, since they don't need to be decoded in the main target?

We should unit test the iso8601Formatter by its own and just assert that the encoded String includes factorial seconds:

func testEncodingDateWithISO8601FullFormat() throws {
    let jsonEncoder = JSONEncoder()
    jsonEncoder.dateEncodingStrategy = .formatted(iso8601Formatter)

    let knownDate: Date = .mockDecember15th2019At10AMUTC(addingTimeInterval: 0.123)
    let encodedKnownDate = try jsonEncoder.encode(knownDate)

    let jsonDecoder = JSONDecoder()
    let knownDateDecodedString = try jsonDecoder.decode(String.self, from: encodedKnownDate)

    XCTAssertEqual(knownDateDecodedString, "2019-12-15T10:00:00.123")
}

Higher-level tests should just work, because:

  • we only have a few precisely picked unit tests which assert the date format - with LoggerTests.testSendingMinimalLogWithDefaultLogger() being the most crucial one;
  • they work against .mockDecember15th2019At10AMUTC() mock, which doesn't include factorial seconds.

@buranmert buranmert force-pushed the buranmert/RUMM-437-iso8601-miliseconds-fix branch from f67b725 to 3026b9a Compare May 5, 2020 09:47
@buranmert
Copy link
Contributor Author

@ncreated I was thinking of a more complete encoding testing strategy (not only Dates) but I couldn't come up with any idea
I guess the test case that you suggested should be fine 👌

@buranmert buranmert force-pushed the buranmert/RUMM-437-iso8601-miliseconds-fix branch from 3026b9a to f9a6884 Compare May 5, 2020 10:00
Copy link
Collaborator

@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 9beeb56 into master May 5, 2020
@buranmert buranmert deleted the buranmert/RUMM-437-iso8601-miliseconds-fix branch May 5, 2020 12:16
@ncreated ncreated removed this from the next-version milestone May 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No sub-second precision in logs
2 participants