fix(contrib/metric/otelmetric): Fix MustProvider function parameter and improve runtime version retrieval#4729
fix(contrib/metric/otelmetric): Fix MustProvider function parameter and improve runtime version retrieval#4729H4x3rs wants to merge 3 commits intogogf:masterfrom
Conversation
- 将metric.WithReader替换为otelmetric.WithReader以匹配正确的函数签名
- 添加 reflect 包导入以支持动态调用 - 实现 getRuntimeVersion 函数，通过反射安全获取运行时版本 - 替换直接调用 runtime.Version() 为 getRuntimeVersion() 调用 - 提高版本获取的稳定性和兼容性
There was a problem hiding this comment.
Pull request overview
This PR updates the otelmetric contrib module to (1) correct the README example to use the module’s Option helpers, and (2) make built-in runtime metrics scope version retrieval resilient to upstream API differences.
Changes:
- Update README example to pass
otelmetric.WithReader(...)intootelmetric.MustProvider(...). - Introduce
getRuntimeVersion()and replace directruntime.Version()calls when constructing built-in metric views.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| contrib/metric/otelmetric/otelmetric_provider.go | Adds a compatibility runtime version getter and uses it for instrumentation scope version in built-in metric views. |
| contrib/metric/otelmetric/README.MD | Fixes the README example to use otelmetric.WithReader (matching MustProvider(...Option)). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func getRuntimeVersion() string { | ||
| val := reflect.ValueOf(runtime.Version) | ||
|
|
||
| if val.Kind() == reflect.Func { | ||
| results := val.Call(nil) | ||
| if len(results) > 0 && results[0].Kind() == reflect.String { |
There was a problem hiding this comment.
getRuntimeVersion uses reflect.Value.Call, which adds overhead and is harder to reason about than necessary. Since the goal is to support both runtime.Version being a string and a func() string, consider using a type-switch on any(runtime.Version) (or interface{}) instead of reflection; it’s simpler and avoids reflect entirely.
| Name: "process.runtime.go.gc.pause_ns", | ||
| Scope: instrumentation.Scope{ | ||
| Name: runtime.ScopeName, | ||
| Version: runtime.Version(), | ||
| Version: getRuntimeVersion(), | ||
| }, |
There was a problem hiding this comment.
getRuntimeVersion() is invoked twice during createViewsForBuiltInMetrics(). If you keep the reflective implementation, consider calling it once and reusing the result to avoid repeating the reflection work.
| // OpenTelemetry provider. | ||
| provider := otelmetric.MustProvider(metric.WithReader(exporter)) | ||
| provider := otelmetric.MustProvider(otelmetric.WithReader(exporter)) | ||
| provider.SetAsGlobal() |
There was a problem hiding this comment.
In the README example, after switching to otelmetric.WithReader(exporter), the go.opentelemetry.io/otel/sdk/metric import in the snippet is no longer used, so the sample code won’t compile as-is. Consider removing that import (or otherwise using it) to keep the example buildable.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
docs(otelmetric/README.MD): 修复MustProvider函数参数错误
fix(otelmetric): 兼容不同版本runtime版本获取逻辑