OTEL tweaks

[ash2k]

First commit: it's good to avoid mutable public fields because they encourage poor user behavior. It's possible to configure the histogram buckets via options, no need to make the default value mutable.

Second commit: it's better to fail early if OTEL library returned an error rather than discard an error, store nil in the field and then panic with NPE later. There will be no way to find out why the field was nil because the error was discarded. Also, it'd be better if WithClient() just returned an error.

Third commit: Second() does something weird instead of just division. It loses precision that way. OpenTelementry libraries just use division, e.g. https://github.com/open-telemetry/opentelemetry-go-contrib/blob/8e44cea6d4fe181343500b6cb680bd501b3d24aa/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go#L343-L344

Overall I think it's not the best choice to have both WithClient() and NewClient(). Why not have a single function and let the user configure if they want metrics, traces, both, and with what options. If we want to keep two functions, names are really not clear. They should be WithTracing() and WithMetrics() or something like that.

But IMO it's better to have a single function. If someone doesn't want e.g. metrics, they can provide a NOOP MeterProvider. Same with tracing - there is a NOOP TracerProvider.

[codecov-commenter]

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (8124203) 96.00% compared to head (5479ac3) 95.96%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
rueidisotel/metrics.go	34.78%	12 Missing and 3 partials ⚠️
rueidisotel/trace.go	33.33%	1 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #428      +/-   ##
==========================================
- Coverage   96.00%   95.96%   -0.05%     
==========================================
  Files          77       77              
  Lines       32392    32403      +11     
==========================================
- Hits        31099    31095       -4     
- Misses       1103     1113      +10     
- Partials      190      195       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rueian]

Hi @ash2k,

Third commit: Second() does something weird instead of just division. It loses precision that way. OpenTelementry libraries just use division.

Thank you for the reference. Good to know this fun fact.

But IMO it's better to have a single function. If someone doesn't want e.g. metrics, they can provide a NOOP MeterProvider. Same with tracing - there is a NOOP TracerProvider.

Totally agreed. I forgot to mark WithClient as deprecated when introducing the new NewClient. Would you like to help me mark WithClient as deprecated?

[ash2k]

Done!

[rueian]

Thanks @ash2k!