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

Daily transactions chart #195

Merged
merged 1 commit into from
Mar 15, 2023
Merged

Daily transactions chart #195

merged 1 commit into from
Mar 15, 2023

Conversation

lubej
Copy link
Collaborator

@lubej lubej commented Mar 10, 2023

Problem

Daily duration changes in #193

Solution

Group buckets by hour

@lubej lubej requested review from buberdds and lukaw3d March 10, 2023 11:42
@lubej lubej force-pushed the ml/daily-transactions-chart branch from 895b85f to c9ff474 Compare March 10, 2023 11:48
@cloudflare-pages
Copy link

cloudflare-pages bot commented Mar 10, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: c400d5b
Status: ✅  Deploy successful!
Preview URL: https://84d8000f.oasis-explorer.pages.dev
Branch Preview URL: https://ml-daily-transactions-chart.oasis-explorer.pages.dev

View logs

src/app/utils/chart-utils.ts Outdated Show resolved Hide resolved
src/app/utils/chart-utils.ts Outdated Show resolved Hide resolved

const chunkArray = buckets.reduce(
(accArray, item, index) => {
const chunkIndex = Math.floor(index / numOfChunks)
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we group by hour, something like isSameHour ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was looking into active accounts chart, which has the "offset" as well. It would make more sense. Should I add it to Active accounts chart as well?

Copy link
Contributor

@buberdds buberdds Mar 10, 2023

Choose a reason for hiding this comment

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

It's not the same. In active accounts we show every 12th items and we do not group/sum them as windows overlaps for 5 minutes buckets.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, after talking with Don, summing by hour makes the most sense.

@lubej lubej force-pushed the ml/daily-transactions-chart branch from c9ff474 to ba090c1 Compare March 10, 2023 13:03
src/app/utils/chart-utils.ts Outdated Show resolved Hide resolved
const totalTransactions = data?.data.buckets.reduce((acc, curr) => acc + curr.tx_volume, 0) ?? 0

const lineChartData = isDailyChart
? sumBucketsByChucks(buckets, 'volume_per_second', 'bucket_start', 12)
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing the reasoning for this PR and #193 is #193 (comment):

hourly buckets won't be added to API

but is that just in MVP - and these are temporary workarounds, or actually won't be? Sounds like misaligned product/mocks/backend.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apparently backend will not support hours buckets. We can discuss in more detail today.

@lubej lubej force-pushed the ml/daily-transactions-chart branch 2 times, most recently from 14432c9 to 8469fb1 Compare March 14, 2023 09:37
@lubej lubej requested review from buberdds and lukaw3d March 14, 2023 09:41
@lubej lubej force-pushed the ml/daily-transactions-chart branch from 8469fb1 to 93788fc Compare March 14, 2023 09:44
@lubej lubej force-pushed the ml/daily-transactions-chart branch from 93788fc to c400d5b Compare March 15, 2023 07:55
@lubej lubej merged commit 491bd93 into master Mar 15, 2023
@lubej lubej deleted the ml/daily-transactions-chart branch March 15, 2023 09:23
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.

None yet

3 participants