-
Notifications
You must be signed in to change notification settings - Fork 852
Add otel metric utils provider #2443
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
Conversation
utils/metrics/metrics_util.go
Outdated
) | ||
|
||
func SetupOtelMetricsProvider() error { | ||
metricsExporter, err := prometheus.New(prometheus.WithNamespace("seidb")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that namespace make sense here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah, need to fix it, good catch!
sei-cosmos/baseapp/baseapp.go
Outdated
// Bind OTEL metrics provider | ||
err := metricutils.SetupOtelMetricsProvider() | ||
if err != nil { | ||
logger.Error(err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious why log the error instead of erroring out. We probably don't want this failure to be scilent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanna be cautious here since it's something new, here we don't panic because I don't wanna sei-chain to crash just due to this new metric provider. We can switch to error out later in the future when we refactor the metrics framework
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (47.21%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2443 +/- ##
==========================================
- Coverage 51.48% 51.46% -0.02%
==========================================
Files 1541 1541
Lines 156492 156504 +12
==========================================
- Hits 80572 80549 -23
- Misses 69833 69859 +26
- Partials 6087 6096 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Describe your changes and provide context
This PR is adding otel metrics provider so that we can add otel metrics in other sub modules.
Remove duplicate integration test.
Testing performed to validate your change