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

chore: update error metrics report #68

Merged
merged 15 commits into from
Mar 5, 2024

Conversation

duyhungtnn
Copy link
Collaborator

@duyhungtnn duyhungtnn commented Feb 27, 2024

Part of bucketeer-io/bucketeer#799
Related to #65

TODO

  • Track the 3xx status code as "RedirectRequestException".
    • add response_code via labels
  • Track the 413 status code as "PayloadTooLargeException"
  • Track 408 status code, and we should handle it as a timeout error.
  • Make iOS error handling consistent with Android SDK. Android handles more errors code than iOS SDK. That is ( 405, 502, 504)
  • Implement adding more information via labels for Unknown errors, so we don't spend too much time debugging.
    • Add error_message and response_code to the ErrorMetricsEvent labels

Tests

  • Track the 3xx status code as "RedirectRequestException"
    • add response_code via labels
  • Track the 413 status code as "PayloadTooLargeException"
  • 408 errors, and we should handle it as a timeout error.
  • Make iOS error handling consistent with Android SDK.
  • Implement adding more information via labels for Unknown errors, so we don't spend too much time debugging.
    • Add error_message and response_code to the ErrorMetricsEvent labels

@duyhungtnn duyhungtnn self-assigned this Feb 27, 2024
@duyhungtnn duyhungtnn marked this pull request as draft February 27, 2024 16:05
@duyhungtnn duyhungtnn force-pushed the chore/update-errors-metrics-report branch from 56ac72e to 2df7258 Compare February 27, 2024 16:07
@duyhungtnn duyhungtnn changed the title chore: update errors metrics report chore: update error metrics report Feb 28, 2024
@duyhungtnn duyhungtnn marked this pull request as ready for review February 29, 2024 14:17
@duyhungtnn
Copy link
Collaborator Author

hi, @kakcy please help me to take a look at this PR. Let me know if you see any problems or questions.
I will continue checking to add more tests if needed.
/cc @cre8ivejp

Copy link
Contributor

@kakcy kakcy left a comment

Choose a reason for hiding this comment

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

thank you!
I have made some comments, please check them.

@@ -250,18 +268,20 @@ extension BKTError: CaseIterable {
}

public static var allCases: [BKTError] = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I think .invalidHttpMethod should be added to this array, what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed in 3ec923d

timeoutMillis: 100) { (result: Result<(MockResponse, URLResponse), Error>) in
switch result {
case .success((let response, _)):
XCTAssertEqual(response, mockResponse)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it will be called here, so I think XCTFail() is better, what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed in 8501d05

@duyhungtnn
Copy link
Collaborator Author

Thank @kakcy for reviewing, let me update the PR follow your suggestion.

@duyhungtnn duyhungtnn requested a review from kakcy March 2, 2024 00:47
@duyhungtnn
Copy link
Collaborator Author

Hi @kakcy please help me to review again. I updated the PR.

Copy link
Contributor

@kakcy kakcy left a comment

Choose a reason for hiding this comment

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

Sorry for the late review.
I reviewed it and it seems to be fine.

thank you 👍

Copy link
Member

@cre8ivejp cre8ivejp left a comment

Choose a reason for hiding this comment

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

Looks good to me!
Thank you!

@cre8ivejp cre8ivejp merged commit 4092c00 into main Mar 5, 2024
8 checks passed
@cre8ivejp cre8ivejp deleted the chore/update-errors-metrics-report branch March 5, 2024 11:29
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