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

GODRIVER-3095 Add moving STD to RTT Stats #1845

Merged
merged 8 commits into from
Oct 8, 2024

Conversation

joyjwang
Copy link
Contributor

@joyjwang joyjwang commented Oct 2, 2024

GODRIVER-3095

Summary

Background & Motivation

# This is the 1st commit message:

test

# This is the commit message mongodb#2:

test2
Copy link
Contributor

mongodb-drivers-pr-bot bot commented Oct 2, 2024

API Change Report

No changes found!

@mongodb-drivers-pr-bot mongodb-drivers-pr-bot bot added the priority-3-low Low Priority PR for Review label Oct 2, 2024
@joyjwang joyjwang marked this pull request as ready for review October 3, 2024 21:05
prestonvasquez
prestonvasquez previously approved these changes Oct 7, 2024
Copy link
Collaborator

@prestonvasquez prestonvasquez left a comment

Choose a reason for hiding this comment

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

LGTM 🔧

"time"
)

func StandardDeviationList[T time.Duration | float64](l *list.List) float64 {
Copy link
Collaborator

@matthewdale matthewdale Oct 8, 2024

Choose a reason for hiding this comment

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

There's currently no use case for this function to be outside of the x/mongo/driver/topology package, so it should be moved there and un-exported. There's also currently no use case for passing a list of float64, so the function can assume the list contains time.Duration and doesn't need to be generic.

Those changes allow us to avoid passing list.List between package APIs, which can lead to runtime panics since list.List doesn't provide compile-time type safety (i.e. Element.Value is type any). When we need a more flexible standard deviation function, we can move it to a shared package and figure out a type-safe way to pass values then.

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 notable that if we don't keep stats functions in a standardized place, duplication is likely.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's possible, but that's easy to fix if it happens. Using a *list.List to pass data of a particular type is prone to runtime panics because of the lack of type safety, so if we were going to generalize the implementation, I'd recommend changing the data type. Keeping it private for now means we don't have to figure that out yet.

BTW I apologize for the conflicting feedback that @prestonvasquez and I gave concerning where to put the stddev implementation.

Copy link
Collaborator

@matthewdale matthewdale left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@joyjwang joyjwang merged commit 24153e5 into mongodb:master Oct 8, 2024
27 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-3-low Low Priority PR for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants