Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added LeveledMeterProvider to telemetry setting #6173

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

chahatsagarmain
Copy link
Contributor

Which problem is this PR solving?

How was this change tested?

  • make lint test

Checklist

Signed-off-by: chahatsagarmain <[email protected]>
Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.51%. Comparing base (f0a1776) to head (1604786).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6173      +/-   ##
==========================================
+ Coverage   96.49%   96.51%   +0.02%     
==========================================
  Files         354      354              
  Lines       20127    20134       +7     
==========================================
+ Hits        19421    19433      +12     
+ Misses        522      518       -4     
+ Partials      184      183       -1     
Flag Coverage Δ
badger_v1 8.31% <0.00%> (ø)
badger_v2 1.67% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v1 14.39% <0.00%> (ø)
cassandra-4.x-v2 1.61% <0.00%> (-0.01%) ⬇️
cassandra-5.x-v1 14.39% <0.00%> (ø)
cassandra-5.x-v2 1.61% <0.00%> (-0.01%) ⬇️
elasticsearch-6.x-v1 18.60% <0.00%> (ø)
elasticsearch-7.x-v1 18.68% <0.00%> (ø)
elasticsearch-8.x-v1 18.85% <0.00%> (+<0.01%) ⬆️
elasticsearch-8.x-v2 1.67% <0.00%> (-0.01%) ⬇️
grpc_v1 9.46% <57.14%> (-0.02%) ⬇️
grpc_v2 6.99% <0.00%> (-0.02%) ⬇️
kafka-v1 8.88% <0.00%> (ø)
kafka-v2 1.67% <0.00%> (-0.01%) ⬇️
memory_v2 1.67% <0.00%> (-0.01%) ⬇️
opensearch-1.x-v1 18.73% <0.00%> (+<0.01%) ⬆️
opensearch-2.x-v1 18.72% <0.00%> (ø)
opensearch-2.x-v2 1.67% <0.00%> (+<0.01%) ⬆️
tailsampling-processor 0.46% <0.00%> (-0.01%) ⬇️
unittests 95.43% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@chahatsagarmain
Copy link
Contributor Author

@mahadzaryab1 how will i propogate leveledmeterprovider where telset is not part of function arguement ? Do i just use noop meterprovider there ?

@mahadzaryab1
Copy link
Collaborator

@mahadzaryab1 how will i propogate leveledmeterprovider where telset is not part of function arguement ? Do i just use noop meterprovider there ?

Yes, it will not be available in the context of v1 so a noop provider can be used there.

Copy link
Collaborator

@mahadzaryab1 mahadzaryab1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work - just a couple of nits

cmd/jaeger/internal/extension/jaegerquery/server.go Outdated Show resolved Hide resolved
pkg/telemetery/settings.go Outdated Show resolved Hide resolved
Signed-off-by: chahatsagarmain <[email protected]>
Copy link
Collaborator

@mahadzaryab1 mahadzaryab1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm but need to wait for @yurishkuro's approval

@yurishkuro
Copy link
Member

yurishkuro commented Nov 8, 2024

I don't think this is addressing all places

$ rg otel/metric/noop
cmd/query/app/server.go
22:	"go.opentelemetry.io/otel/metric/noop"

plugin/storage/grpc/factory.go
21:	noopmetric "go.opentelemetry.io/otel/metric/noop"

cmd/collector/app/handler/otlp_receiver.go
24:	noopmetric "go.opentelemetry.io/otel/metric/noop"

cmd/collector/app/handler/zipkin_receiver.go
16:	noopmetric "go.opentelemetry.io/otel/metric/noop"

@mahadzaryab1
Copy link
Collaborator

mahadzaryab1 commented Nov 8, 2024

@yurishkuro (1) looks to be addressed in this PR. (3) and (4) are v1 components so we're passing in noop (https://github.com/jaegertracing/jaeger/blob/main/cmd/collector/app/handler/otlp_receiver.go#L71-L81). Is there anything else we can do for that?

@chahatsagarmain for (2) We can pass the meter provider down from this constructor and into the grpc factory.

Copy link
Collaborator

@mahadzaryab1 mahadzaryab1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one more comment

plugin/storage/grpc/factory.go Show resolved Hide resolved
Comment on lines 66 to 69
metricsFactory metrics.Factory,
logger *zap.Logger,
host component.Host,
meterProvider metric.MeterProvider,
Copy link
Member

@yurishkuro yurishkuro Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the opportunity to simplify the signature:

Suggested change
metricsFactory metrics.Factory,
logger *zap.Logger,
host component.Host,
meterProvider metric.MeterProvider,
telset telemetry.Settings,

chahatsagarmain and others added 3 commits November 14, 2024 21:12
Co-authored-by: Yuri Shkuro <[email protected]>
Signed-off-by: chahat sagar <[email protected]>
Signed-off-by: chahatsagarmain <[email protected]>
host component.Host,
metricsFactory metrics.Factory,
telset component.TelemetrySettings,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant Jaeger's internal telemetry.Settings, which includes both host and metrics factory

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

telset := telemetery.Setting{ Logger: s.telset.Logger, Host: host, Metrics: mf, LeveledMeterProvider: func(_ configtelemetry.Level) metric.MeterProvider { return s.telset.MeterProvider }, }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can i pass meter provider like this ? or should i have a field to pass meter provider in function signature ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes that's fine (ie to ignore the level)

Signed-off-by: chahatsagarmain <[email protected]>
@chahatsagarmain
Copy link
Contributor Author

@yurishkuro spm test is failing for recent prs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[jaeger-v2] Add Meter Provider To Telemetry Settings
3 participants