Improve telemetry.Settings #6275
Conversation
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6275 +/- ##
==========================================
+ Coverage 96.41% 96.44% +0.03%
==========================================
Files 355 355
Lines 20152 20165 +13
==========================================
+ Hits 19430 19449 +19
+ Misses 532 528 -4
+ Partials 190 188 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
| LeveledMeterProvider: func(_ configtelemetry.Level) metric.MeterProvider { | ||
| return noop.NewMeterProvider() |
There was a problem hiding this comment.
should this be replaced with a meter provider? or do we not need to pass anything here because its noop?
There was a problem hiding this comment.
I added noop to L100, but afaik we don't have a way to go from internal metrics.Factory to OTEL API. We would need to extend the Service (cmd/internal/flags) to initialize OTEL SDK. Main reason I didn't push for that before is that OTEL SDK performance sucks compared to Prometheus SDK, since OTEL does not support bound instrument and we're using them everywhere.
| assert.NotNil(t, telset.TracerProvider) | ||
| assert.NotNil(t, telset.ReportStatus) | ||
| assert.NotNil(t, telset.Host) | ||
| telset.ReportStatus(componentstatus.NewFatalErrorEvent(errors.New("foobar"))) |
There was a problem hiding this comment.
what do we need this for?
There was a problem hiding this comment.
you mean in this test or in general? In this test it's just to exercise the code in the lambda. In general, the idea is to allow internal components to report status. We had some of it with healthcheck object, but it's not very comprehensive today.
| @@ -18,25 +18,30 @@ import ( | |||
| "github.com/jaegertracing/jaeger/storage/spanstore" | |||
| ) | |||
|
|
|||
| // Same as Factory, but without the Initialize method. | |||
| // It was a design mistake originally to add Initialize to the Factory interface. | |||
| type BaseFactory interface { | |||
There was a problem hiding this comment.
do we have a longer term plan to get rid of Factory and replace it with this?
There was a problem hiding this comment.
I don't know if it's worth it for v1. We should make sure v2 API is not repeating the same mistake.
Signed-off-by: Yuri Shkuro <[email protected]>
Which problem is this PR solving?
Description of the changes
telemetry.Settingtotelemetry.SettingsNoopSettings()andFromOtelComponent()LeveledMeterProviderwhich is deprecated in OTELtelsetin more placesInitialize()fromstorage.FactoryHow was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test