Fix initialization of the MetricProvider #5571
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5571 +/- ##
==========================================
- Coverage 91.28% 91.27% -0.01%
==========================================
Files 191 191
Lines 11302 11304 +2
==========================================
+ Hits 10317 10318 +1
- Misses 784 785 +1
Partials 201 201
Continue to review full report at Codecov.
|
| @@ -133,6 +133,7 @@ func TestCollectorReportError(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestCollectorFailedShutdown(t *testing.T) { | |||
| t.Skip("This test was using telemetry shutdown failure, switch to use a component that errors on shutdown.") | |||
There was a problem hiding this comment.
Is this unnecessary now? Maybe delete it?
There was a problem hiding this comment.
As you can see it is a TODO, to use a different design, and re-enable. Not sure how to do this without blowing the size of this PR.
service/service.go
Outdated
| if err = srv.colTelemetry.init(set.BuildInfo, srv.telemetry.Logger, set.Config.Service.Telemetry, set.AsyncErrorChannel); err != nil { | ||
| return nil, fmt.Errorf("failed to initialize telemetry: %w", err) | ||
| } | ||
| srv.telemetry.MeterProvider = srv.colTelemetry.mp |
There was a problem hiding this comment.
Can we return this from colTelemetry.init()? Generally, I think we should avoid working with the private fields of colTelemetry here. Perhaps to enforce it move colTelemetry to a separate package.
There was a problem hiding this comment.
Will do that, when moving to service/telemetry package, in a soon followup PR.
bogdandrutu
force-pushed
the
fixmetricproviderinit
branch
4 times, most recently
from
459f068 to
e68ad80
Compare
bogdandrutu
force-pushed
the
fixmetricproviderinit
branch
from
e68ad80 to
e8005ac
Compare
The problem was that the MetricProvider is initialized into the "service.telemetry.MetricProvider" after components were created. This change was not a trivial change because the process telemetry initialization requires the ballast size, which is available after the extensions are initialized, because of that I split the initialization of the MetricProvider/oc.Registry from the initialization of the process telemetry. Signed-off-by: Bogdan Drutu <[email protected]>
bogdandrutu
force-pushed
the
fixmetricproviderinit
branch
from
e8005ac to
8255a54
Compare
The problem was that the MetricProvider is initialized into the "service.telemetry.MetricProvider" after components were created, so all components will get a no-op implementation even if the feature gate is enabled. This change was not a trivial change because the process telemetry initialization requires the ballast size, which is available after the extensions are initialized, because of that I split the initialization of the MetricProvider/oc.Registry from the initialization of the process telemetry.
Signed-off-by: Bogdan Drutu [email protected]