otelgrpc: stats handlers record durations in ms instead of ns

[Sovietaced]

This pull request fixes an issue recently introduced in #4356.

I believe it was unintentional that durations would be recorded using nanoseconds when the histogram is a float64. The code is now aligned with how http metric durations are recorded.

Fixes issue: #4547

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Sovietaced / name: Jason Parraga (d36f189, a454728, 007d252, 4b3e369)
• ✅ login: pellared / name: Robert Pająk (007d252, 445c84d, 3393623)
• ✅ login: MrAlias / name: Tyler Yahn (744c767)

[codecov]

Codecov Report

Merging #4548 (3393623) into main (d9e86fb) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #4548   +/-   ##
=====================================
  Coverage   80.9%   80.9%           
=====================================
  Files        150     150           
  Lines      10342   10348    +6     
=====================================
+ Hits        8369    8375    +6     
  Misses      1831    1831           
  Partials     142     142           

Files	Coverage Δ
...ion/google.golang.org/grpc/otelgrpc/interceptor.go	88.5% <100.0%> (+<0.1%)	⬆️
...n/google.golang.org/grpc/otelgrpc/stats_handler.go	92.7% <100.0%> (+0.2%)	⬆️

[Sovietaced]

Is it too much overhead for this to make it into a patch? We had been eagerly waiting the latest release for the stats handler metrics.

[MrAlias]

Is it too much overhead for this to make it into a patch? We had been eagerly waiting the latest release for the stats handler metrics.

Are you not able to take a dependency on the git hash when this merges?

[Sovietaced]

Are you not able to take a dependency on the git hash when this merges?

Yeah we should! Sorry I'm new to Go

[vroldanbet]

Excuse the drive-by comment 👋🏻

Security scanning tools are flagging go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc@v0.45.0 with CVE, so folks are being prompted to update to 0.46.0. The new release also introduces the deprecation of a bunch of interceptor factory methods, and because docs didn't clarify how to move to stats.Handler, I started looking into the code, which led me to this issue. For reference the CVE:

┌──────────────────────────────────────────────────────────────┬────────────────┬──────────┬────────┬───────────────────┬───────────────┬───────────────────────────────────────────────────────┐
│                           Library                            │ Vulnerability  │ Severity │ Status │ Installed Version │ Fixed Version │                         Title                         │
├──────────────────────────────────────────────────────────────┼────────────────┼──────────┼────────┼───────────────────┼───────────────┼───────────────────────────────────────────────────────┤
│ go.opentelemetry.io/contrib/instrumentation/google.golang.o- │ CVE-2023-47108 │ HIGH     │ fixed  │ 0.45.0            │ 0.46.0        │ otelgrpc DoS vulnerability due to unbound cardinality │
│ rg/grpc/otelgrpc                                             │                │          │        │                   │               │ metrics                                               │
│                                                              │                │          │        │                   │               │ https://avd.aquasec.com/nvd/cve-2023-47108            │
└──────────────────────────────────────────────────────────────┴────────────────┴──────────┴────────┴───────────────────┴───────────────┴───────────────────────────────────────────────────────┘

The title of this PR and its accompanying bug report seemed scary as it would mean starting to report several orders of magnitude more milliseconds. I wrote a test that compared the previous code and the proposed code, and I couldn't see a difference other than the resolution of the float, but the unit seems correct. Am I missing something? If the goal is to increase the resolution of the float, then the bug and PR description should be updated.

Here is a small snippet showing it: https://go.dev/play/p/oF4t3c9wBDZ

[dashpole]

I do think this justifies a patch release given the recent CVE

[Sovietaced]

The title of this PR and its accompanying bug report seemed scary as it would mean starting to report several orders of magnitude more milliseconds. I wrote a test that compared the previous code and the proposed code, and I couldn't see a difference other than the resolution of the float, but the unit seems correct. Am I missing something? If the goal is to increase the resolution of the float, then the bug and PR description should be updated.

If you look at the original commit you'll see a time.Duration being casted to a float64: d36f189

The other changes were just done as follow ups to comments about improving the resolution of the float.

[vroldanbet]

If you look at the original commit you'll see a time.Duration being casted to a float64: d36f189

🤦🏻 I missed that the problem was indeed in the stats handler, and I had only looked into the interceptor. All good!

[pellared]

If you look at the original commit you'll see a time.Duration being casted to a float64: d36f189

🤦🏻 I missed that the problem was indeed in the stats handler, and I had only looked into the interceptor. All good!

I updated the changelog to make it clear. You can se that your feedback was still valuable 👍

[pellared]

@Sovietaced Thank you for your contribution 👍