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

gRPC metrics record duration in nanoseconds instead of milliseconds #4547

Closed
Sovietaced opened this issue Nov 10, 2023 · 6 comments
Closed
Labels
area: instrumentation Related to an instrumentation package bug Something isn't working instrumentation: otelgrpc

Comments

@Sovietaced
Copy link
Contributor

Description

RPC metrics are recording float64 of nanoseconds instead of milliseconds as they are intended to.

Expected behavior

If a float is going to be used to record RPC durations (similar to what is done for HTTP durations) we should be recording milliseconds and not nanoseconds.

@Sovietaced Sovietaced added area: instrumentation Related to an instrumentation package bug Something isn't working instrumentation: otelgrpc labels Nov 10, 2023
@Sovietaced Sovietaced changed the title RPC metrics record duration in nanoseconds instead of milliseconds gRPC metrics record duration in nanoseconds instead of milliseconds Nov 10, 2023
@Sovietaced
Copy link
Contributor Author

I will file a pull request for this momentarily.

@MrAlias

This comment was marked as outdated.

@MrAlias

This comment was marked as outdated.

@ash2k
Copy link
Contributor

ash2k commented Nov 17, 2023

Shouldn't all duration metrics be in seconds? I think I saw this requirement/recommendation somewhere in the spec. For histograms this would mean using custom buckets (which is now possible!) as the default ones would not be granular enough for "seconds" resolution.

@MrAlias
Copy link
Contributor

MrAlias commented Nov 17, 2023

Shouldn't all duration metrics be in seconds? I think I saw this requirement/recommendation somewhere in the spec. For histograms this would mean using custom buckets (which is now possible!) as the default ones would not be granular enough for "seconds" resolution.

@ash2k please open an issue to track this independently.

For what it's worth, the change from ms to s happened in later versions of the semantic conventions that we do not currently support. We should track this so we can make sure we include it when we do update.

@ash2k
Copy link
Contributor

ash2k commented Nov 18, 2023

@MrAlias Opened #4590. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: instrumentation Related to an instrumentation package bug Something isn't working instrumentation: otelgrpc
Projects
None yet
Development

No branches or pull requests

3 participants