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

fix(stats): populate empty chart axis #289

Merged
merged 15 commits into from
Jun 29, 2023

Conversation

mbhrznr
Copy link
Contributor

@mbhrznr mbhrznr commented Jun 22, 2023

closes #276.

sets the y-axis' ticks to be of natural numbers.
populates the x-axis with sequential dates.

to prevent us from having "holes" in the retrieved dates, a new getDatesSince has been introduced.
the returned array can then can be used to query the necessary fields from our analytics stores.

Bildschirm­foto 2023-06-22 um 23 42 37

Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

This PR mostly tweaks spacing to adhere to a particular stylistic preference. Can you revert these changes so the PR only targets the main objective and keeps the diff trim?

@mbhrznr
Copy link
Contributor Author

mbhrznr commented Jun 23, 2023

sorry about the initial formatting mess, prettier was still active.

utils/db.ts Outdated
const dates = Array.from(metricsMap.keys());
const metricsValue = Array.from(metricsMap.values());

return { dates, metricsValue };
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's difficult to understand what's going on here. If possible, we can break this into more straightforward, smaller functions and remove unnecessary moving parts. I'm open to ideas. It'd also be good to have this tested.

Copy link
Contributor Author

@mbhrznr mbhrznr Jun 25, 2023

Choose a reason for hiding this comment

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

thanks for the feedback.
there was indeed quite a lot going on there.

i refactored the logic to be more aligned w/ existing functions which were expecting a msAgo argument.
i also introduced some small helpers:

  • getDatesSince to get all dates since a given number of milliseconds ago
  • formatDate to have a unified way of formatting dates

with the newly created tests it should be more predictable to see what's going on.
however i'd still await the changes introduced in #296.

@iuioiua
Copy link
Contributor

iuioiua commented Jun 28, 2023

#296 has been merged. PTAL, @mbhrznr, as it has some overlap with this PR.

@mbhrznr mbhrznr force-pushed the fix/populate-empty-chart-axis branch from 3d729f9 to 3c3c5da Compare June 28, 2023 05:07
@mbhrznr
Copy link
Contributor Author

mbhrznr commented Jun 28, 2023

@iuioiua thanks for the heads-up!
pr has been updated accordingly.

@brunocorrea23 great job on #296! 👏 👏

@mbhrznr
Copy link
Contributor Author

mbhrznr commented Jun 28, 2023

following on the discussions of #296:
would it make sense to refactor our current getAll[X]CountsByDay functions to a getAll[X]CountsSince variant?
if we'd refactor /stats to make use of the current implementation we'd still have the issue that the chart axis is (potentially) populated in a non-sequential manner.

@iuioiua
Copy link
Contributor

iuioiua commented Jun 28, 2023

Interesting. My current issue with stats and some of DB is that I think the functions do too much, respectively, though we're slowly getting better. E.g. I'd prefer hardcoding the respective metric instead of having a metric parameter in getAnalyticsMetricListPerDay(), and having a greater number of smaller, simpler functions.

Regarding your suggestion, what if we did this, using getDatesSince()?

Before:

export async function getAllVisitsCountByDay(options?: Deno.KvListOptions) {
  const iter = await kv.list<bigint>({ prefix: ["visits_count"] }, options);
  const visits = [];
  const dates = [];
  for await (const res of iter) {
    visits.push(Number(res.value));
    dates.push(String(res.key[1]));
  }
  return { visits, dates };
}

const visitsCount = await getAllVisitsCountByDay();

After:

export async function getManyVisitsCounts(dates: string[]) {
  const keys = dates.map((date) => ["visits_count", date]);
  const res = await kv.getMany<bigint[]>(keys);
  return res.map((value) => value);
}

const dates = getDatesSince(DAY * 2);
const visitsCount = await getManyVisitsCounts(dates);

This seems more streamlined, IMO. WDYT?

@iuioiua
Copy link
Contributor

iuioiua commented Jun 28, 2023

I'd be happy to make the above simplifications to this or another PR to help you if you'd like.

@mbhrznr
Copy link
Contributor Author

mbhrznr commented Jun 28, 2023

thanks for the recommendation, i really like that one!
i'd incorporate the changes in couple hours from now

@iuioiua
Copy link
Contributor

iuioiua commented Jun 29, 2023

#312 has been merged. Please feel free to carry on with this PR now.

@mbhrznr mbhrznr force-pushed the fix/populate-empty-chart-axis branch from 5118fd0 to cb72fe1 Compare June 29, 2023 06:11
@mbhrznr
Copy link
Contributor Author

mbhrznr commented Jun 29, 2023

@iuioiua thanks for the most recent changes!

bumped the limit for the metrics back to 30, while also adding a small getMany helper which introduces batching.
most of the initial tests became invalid or were introduced via other prs, which is why i only added two sample cases to test for getMany's length.

Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

Mostly LGTM! Few nits.

utils/db.ts Outdated Show resolved Hide resolved
utils/db.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

I made a few tweaks, and now, LGTM! Thank you for this! Much appreciated.

@iuioiua iuioiua merged commit 9c815af into denoland:main Jun 29, 2023
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.

bug: fix /stats charts axis scale
2 participants