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.

Sign up for GitHub

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

Jump to bottom

Refactor time util and improve diagnostic logging with timezone friendly date and time for Brave Ads events #5444

Merged
merged 1 commit into from
May 8, 2020
Merged

Refactor time util and improve diagnostic logging with timezone friendly date and time for Brave Ads events #5444

merged 1 commit into from
May 8, 2020

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented May 2, 2020

Resolves brave/brave-browser#9199

Submitter Checklist:

Test Plan:

  • Confirm "Next payment date" on ads panel is working as expected

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@tmancey tmancey self-assigned this May 2, 2020
@tmancey tmancey requested a review from NejcZdovc May 2, 2020 01:14
@tmancey tmancey force-pushed the issues/9199 branch 9 times, most recently from 2de515f to 023d050 Compare May 4, 2020 10:52
@tmancey tmancey requested a review from zenparsing May 4, 2020 13:00
zenparsing
zenparsing approved these changes May 4, 2020
View reviewed changes
components/brave_ads/browser/ads_service_impl.cc Show resolved Hide resolved
...-native-ads/src/bat/ads/internal/frequency_capping/exclusion_rules/per_hour_frequency_cap.cc Outdated
@@ -5,7 +5,7 @@

#include "bat/ads/internal/frequency_capping/exclusion_rules/per_hour_frequency_cap.h"
#include "bat/ads/internal/frequency_capping/frequency_capping.h"
#include "bat/ads/internal/time.h"
#include "bat/ads/internal/time_util.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't seem to be using anything defined in time_util.h, is that right? Are we including it just to get base/time/time.h? I see a bunch of similar includes of time_util.h.

vendor/bat-native-ads/src/bat/ads/internal/reports.cc
@@ -31,7 +32,8 @@ std::string Reports::GenerateAdNotificationEventReport(
rapidjson::StringBuffer buffer;
rapidjson::Writer<rapidjson::StringBuffer> writer(buffer);

const std::string timestamp = Time::Timestamp();
const base::Time time = base::Time::Now();
const std::string timestamp = FriendlyDateAndTime(time, false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose this isn't really a "timestamp" anymore, but rather a formatted date string, but it looks like renaming the variable/JSON field would be painful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Logs are being refactored as part of brave/brave-browser#6845 and brave/brave-browser#9533 so reports will be deprecated

vendor/bat-native-confirmations/src/bat/confirmations/internal/confirmations_impl.cc
@@ -1040,8 +1040,13 @@ uint64_t ConfirmationsImpl::GetAdNotificationsReceivedThisMonthForTransactions(
now.UTCExplode(&now_exploded);

for (const auto& transaction : transactions) {
if (transaction.timestamp_in_seconds == 0) {
// Workaround for Windows crash when passing 0 to UTCExplode
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

vendor/bat-native-confirmations/src/bat/confirmations/internal/time_util.cc
@@ -0,0 +1,76 @@
/* Copyright (c) 2020 The Brave Authors. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a shame that we have to duplicate, but 🤷

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Totally agree, and I have raised brave/brave-browser#9611

moritzhaller
moritzhaller approved these changes May 4, 2020
View reviewed changes
Copy link
Contributor

@moritzhaller moritzhaller left a comment

Choose a reason for hiding this comment

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

lgtm!

@tmancey tmancey added CI/skip-android Do not run CI builds for Android CI/skip-linux labels May 4, 2020
@tmancey
Copy link
Collaborator Author

tmancey commented May 4, 2020

CI failed for iOS, macOS and Windows. Restarting

@tmancey tmancey added the CI/skip-ios Do not run CI builds for iOS label May 4, 2020
@tmancey
Copy link
Collaborator Author

tmancey commented May 4, 2020

CI failed for macOS and Windows. Restarting

@tmancey
Copy link
Collaborator Author

tmancey commented May 4, 2020

CI failed macOS, restarting macOS

@tmancey tmancey removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels May 5, 2020
@tmancey
Copy link
Collaborator Author

tmancey commented May 5, 2020

macOS failed on CI, restarting macOS

@tmancey tmancey force-pushed the issues/9199 branch 2 times, most recently from 9fac124 to dc34c5b Compare May 5, 2020 10:09
@tmancey
Copy link
Collaborator Author

tmancey commented May 8, 2020

Failed CI due to known iOS issue and rewards browser tests

@tmancey tmancey merged commit 55cd0df into master May 8, 2020
@tmancey tmancey deleted the issues/9199 branch May 8, 2020 08:54
@tmancey tmancey added this to the 1.10.x - Nightly milestone May 8, 2020
@GeetaSarvadnya GeetaSarvadnya mentioned this pull request Jun 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moritzhaller moritzhaller moritzhaller approved these changes

@zenparsing zenparsing zenparsing approved these changes

@emerick emerick Awaiting requested review from emerick

@gdregalo gdregalo Awaiting requested review from gdregalo

@NejcZdovc NejcZdovc Awaiting requested review from NejcZdovc

Assignees

@tmancey tmancey

Labels
feature/ads
Projects
None yet
Milestone
1.10.x - Release
Development

Successfully merging this pull request may close these issues.

Refactor time util and improve diagnostic logging with timezone friendly date and time for Brave Ads events
3 participants
@tmancey @moritzhaller @zenparsing