Skip to content

feat: Fix Deployment DAUs to work with local timezones #7647

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

Merged
merged 18 commits into from
May 30, 2023
Merged

feat: Fix Deployment DAUs to work with local timezones #7647

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
f7f5d66
chore: Add timezone param to DAU SQL query
Emyrk May 23, 2023
45e8342
Merge DAUs response
Emyrk May 23, 2023
b502940
Pass time offsets to metricscache
Emyrk May 23, 2023
9b970ae
Fix unit tests
Emyrk May 23, 2023
4eb43c1
Add unit tests for tzs to offsets
Emyrk May 23, 2023
1949217
Add unit test for closest
Emyrk May 23, 2023
b5b5824
Return after http error
Emyrk May 23, 2023
448d5fb
make gen
Emyrk May 23, 2023
5976ac4
add comments
Emyrk May 23, 2023
6ff576b
Fix imports
Emyrk May 23, 2023
a91debd
missing import
Emyrk May 23, 2023
0d19300
add unit test
Emyrk May 23, 2023
d7e8b25
Formatting
Emyrk May 23, 2023
bf1bd47
Fix FE to use singulare DAUsResponse
Emyrk May 23, 2023
dc324a8
Fix unit test
Emyrk May 23, 2023
bb7c392
Make gen
Emyrk May 23, 2023
16c9c8f
Format
Emyrk May 24, 2023
a86bcd6
Format
Emyrk May 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Pass time offsets to metricscache
  • Loading branch information
@Emyrk
Emyrk committed May 23, 2023
commit b50294017be17a523661f93482a0140c4fb885d6
2 changes: 1 addition & 1 deletion coderd/insights.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func (api *API) deploymentDAUs(rw http.ResponseWriter, r *http.Request) {
return
}

resp, _ := api.metricsCache.DeploymentDAUs()
_, resp, _ := api.metricsCache.DeploymentDAUs(0)
if resp == nil || resp.Entries == nil {
httpapi.Write(ctx, rw, http.StatusOK, &codersdk.DAUsResponse{
Entries: []codersdk.DAUEntry{},
Expand Down
77 changes: 64 additions & 13 deletions coderd/metricscache/metricscache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"database/sql"
"fmt"
"math"
"sync"
"sync/atomic"
"time"
Expand All @@ -30,8 +31,8 @@ type Cache struct {
log slog.Logger
intervals Intervals

deploymentDAUResponses atomic.Pointer[codersdk.DAUsResponse]
templateDAUResponses atomic.Pointer[map[uuid.UUID]codersdk.DAUsResponse]
deploymentDAUResponses atomic.Pointer[map[int]codersdk.DAUsResponse]
templateDAUResponses atomic.Pointer[map[int]map[uuid.UUID]codersdk.DAUsResponse]
templateUniqueUsers atomic.Pointer[map[uuid.UUID]int]
templateAverageBuildTime atomic.Pointer[map[uuid.UUID]database.GetTemplateAverageBuildTimeRow]
deploymentStatsResponse atomic.Pointer[codersdk.DeploymentStats]
Expand Down Expand Up @@ -161,8 +162,8 @@ func (c *Cache) refreshTemplateDAUs(ctx context.Context) error {
}

var (
deploymentDAUs = codersdk.DAUsResponse{}
templateDAUs = make(map[uuid.UUID]codersdk.DAUsResponse, len(templates))
deploymentDAUs = map[int]codersdk.DAUsResponse{}
templateDAUs = make(map[int]map[uuid.UUID]codersdk.DAUsResponse, len(templates))
templateUniqueUsers = make(map[uuid.UUID]int)
templateAverageBuildTimes = make(map[uuid.UUID]database.GetTemplateAverageBuildTimeRow)
)
Expand All @@ -171,7 +172,7 @@ func (c *Cache) refreshTemplateDAUs(ctx context.Context) error {
if err != nil {
return err
}
deploymentDAUs = convertDAUResponse(rows)
deploymentDAUs[0] = convertDAUResponse(rows)
c.deploymentDAUResponses.Store(&deploymentDAUs)

for _, template := range templates {
Expand All @@ -182,7 +183,7 @@ func (c *Cache) refreshTemplateDAUs(ctx context.Context) error {
if err != nil {
return err
}
templateDAUs[template.ID] = convertDAUResponse(rows)
templateDAUs[0][template.ID] = convertDAUResponse(rows)
templateUniqueUsers[template.ID] = countUniqueUsers(rows)

templateAvgBuildTime, err := c.database.GetTemplateAverageBuildTime(ctx, database.GetTemplateAverageBuildTimeParams{
Expand Down Expand Up @@ -284,26 +285,76 @@ func (c *Cache) Close() error {
return nil
}

func (c *Cache) DeploymentDAUs() (*codersdk.DAUsResponse, bool) {
func (c *Cache) DeploymentDAUs(offset int) (int, *codersdk.DAUsResponse, bool) {
m := c.deploymentDAUResponses.Load()
return m, m != nil
if m == nil {
return 0, nil, false
}
closestOffset, resp, ok := closest(*m, offset)
if !ok {
return 0, nil, false
}
return closestOffset, &resp, ok
}

// TemplateDAUs returns an empty response if the template doesn't have users
// or is loading for the first time.
func (c *Cache) TemplateDAUs(id uuid.UUID) (*codersdk.DAUsResponse, bool) {
// The cache will select the closest DAUs response to given timezone offset.
func (c *Cache) TemplateDAUs(id uuid.UUID, offset int) (int, *codersdk.DAUsResponse, bool) {
m := c.templateDAUResponses.Load()
if m == nil {
// Data loading.
return nil, false
return 0, nil, false
}

resp, ok := (*m)[id]
closestOffset, resp, ok := closest(*m, offset)
if !ok {
// Probably no data.
return nil, false
return 0, nil, false
}

tpl, ok := resp[id]
if !ok {
// Probably no data.
return 0, nil, false
}

return closestOffset, &tpl, true
}

// closest returns the value in the values map that has a key with the value most
// close to the requested key. This is so if a user requests a timezone offset that
// we do not have, we return the closest one we do have to the user.
func closest[V any](values map[int]V, offset int) (int, V, bool) {
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 really understand the purpose of only allowing a set number of timezones. What would be the harm in just passing them to Postgres verbatim?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean by that? There are only 24 timezones, and I just used this instead of input validation because idk if there is some odd edge case of a -13 offset.

As for passing it to postgres. We cache these values and reuse them for all calls. So when the caller requests the data, they pull from a cache, they do not trigger a query. So the data must already be available.

I suppose a better cache would populate things lazily, but that would require a larger refactor.

Copy link
Member

Choose a reason for hiding this comment

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

There are more than 24 time zones, e.g. India is 10.5h ahead of CST.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am doing a +/- 12hr offset to UTC (0). So India would use the +10 bucket if it's +10.5.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, I think I misinterpreted your comment as stating that there are 24 hours timezone in the world. I think an hour error tolerance is immaterial, so am good with the implentation you've described. But, I haven't checked the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that makes sense. I forgot that everything is cached in the background rather than on-demand. It's unfortunate that we need count(templates) * 24 queries for each tick though.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is unfortunate, but it runs once per hour, so I am not too worried.

if len(values) == 0 {
var v V
return -1, v, false
}

v, ok := values[offset]
if ok {
// We have the exact offset, that was easy!
return offset, v, true
}

var closest int
var closestV V
diff := math.MaxInt
for k, v := range values {
if abs(k-offset) < diff {
// new closest
closest = k
closestV = v
}
}
return closest, closestV, true
}

func abs(a int) int {
if a < 0 {
return -1 * a
}
return &resp, true
return a
}

// TemplateUniqueUsers returns the number of unique Template users
Expand Down
2 changes: 1 addition & 1 deletion coderd/templates.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,7 +620,7 @@ func (api *API) templateDAUs(rw http.ResponseWriter, r *http.Request) {
ctx := r.Context()
template := httpmw.TemplateParam(r)

resp, _ := api.metricsCache.TemplateDAUs(template.ID)
_, resp, _ := api.metricsCache.TemplateDAUs(template.ID, 0)
if resp == nil || resp.Entries == nil {
httpapi.Write(ctx, rw, http.StatusOK, &codersdk.DAUsResponse{
Entries: []codersdk.DAUEntry{},
Expand Down