Skip to content

adds stats to ocsql#28

Merged
basvanbeek merged 4 commits into
masterfrom
add-stats
Jan 17, 2019
Merged

adds stats to ocsql#28
basvanbeek merged 4 commits into
masterfrom
add-stats

Conversation

@basvanbeek
Copy link
Copy Markdown
Member

Initial implementation of stats for ocsql.

closes #1

@basvanbeek
Copy link
Copy Markdown
Member Author

PTAL @odeke-em

Copy link
Copy Markdown
Member

@odeke-em odeke-em 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 and nice work @basvanbeek, LGTM with one minor comment.

Comment thread observability.go Outdated
startTime := time.Now()

return func(err error) {
timeSpent := float64(time.Since(startTime).Nanoseconds()) / 1e6
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

timeSpentMs

which will easily give anyone reading this code the context that these are in milliseconds.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

good idea...

Comment thread observability.go
}
}

func recordCallStats(ctx context.Context, method string) func(err error) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice!

Comment thread dbstats_go1.11.go Outdated
}()

return func() {
close(done)
Copy link
Copy Markdown
Member

@odeke-em odeke-em Jan 16, 2019

Choose a reason for hiding this comment

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

Actually what we can do is ensure that this ever gets called once otherwise calling this again will cause a panic and some folks might misuse it.

var closeOnce sync.Once

return func() {
    closeOnce.Do(func() {
      close(done)
    })
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yep, will do...

@basvanbeek basvanbeek merged commit eb3ef1d into master Jan 17, 2019
@basvanbeek basvanbeek deleted the add-stats branch January 17, 2019 07:32
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.

metrics

2 participants