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

multidim-interop: change output units to seconds #127

Closed
wants to merge 1 commit into from

Conversation

marten-seemann
Copy link
Contributor

Let's use SI units wherever possible.

Let's use SI units wherever possible.
@MarcoPolo
Copy link
Contributor

Why seconds? This is subjective, but milliseconds seems to be natural range for this as results will be between between 0.1 and ~100. That reads easier than 0.0001 to 0.100.

@marten-seemann
Copy link
Contributor Author

Any choice will be arbitrary, so I suggest to use SI units. Same reason we measure distances in meters and not in feet 🤓

As a point of reference, Prometheus also has the same convention: https://prometheus.io/docs/practices/naming/

@MarcoPolo
Copy link
Contributor

I don't understand. This is is an SI unit with an SI prefix. I really don't want to change this just to avoid an SI prefix.

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

I am ambivalent. Happy to move forward with this if you own the change through the various repositories @marten-seemann!

Note regarding prometheus: It does recommend a _seconds postfix so it would be "pingRTTSeconds".

@MarcoPolo
Copy link
Contributor

Overall I'm leaning towards closing this. This seems to be minor and don't make the test any better. The cost is extra time updating and backporting these changes. Unless you feel strongly and want to push these changes @marten-seemann, I suggest closing this.

@MarcoPolo
Copy link
Contributor

Closing this as it's been a week. If you're committed to this, feel free to reopen.

@MarcoPolo MarcoPolo closed this Feb 17, 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.

3 participants