-
Notifications
You must be signed in to change notification settings - Fork 108
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
Export internal metrics using OTEL metrics #1425
base: main
Are you sure you want to change the base?
Conversation
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.
LGTM! Sweet!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1425 +/- ##
==========================================
- Coverage 80.97% 71.62% -9.36%
==========================================
Files 146 146
Lines 14843 15043 +200
==========================================
- Hits 12019 10774 -1245
- Misses 2235 3546 +1311
- Partials 589 723 +134
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -7,7 +7,8 @@ import ( | |||
|
|||
// Config options for the different metrics exporters | |||
type Config struct { | |||
Prometheus PrometheusConfig `yaml:"prometheus,omitempty"` | |||
Prometheus PrometheusConfig `yaml:"prometheus,omitempty"` | |||
OTELMetrics bool `yaml:"otel_metrics,omitempty" env:"BEYLA_INTERNAL_METRICS_OTEL_METRICS"` |
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.
Maybe to be less redundant, rename YAML property to otel
or use_otel
and the env var to BEYLA_INTERNAL_METRICS_OTEL
or BEYLA_INTERNAL_METRICS_USE_OTEL
?
Even, to rely less on booleans for config, replace this property for something like protocol
or exporter
, which can be none
, prometheus
or otel
.
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.
Even, to rely less on booleans for config, replace this property for something like
protocol
orexporter
, which can benone
,prometheus
orotel
.
Is not a bad idea, but that would imply breaking changes, no? basically we have to force everyone to set exporter
and prometheus
config for internal metrics.
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 guess we can document that breaking change in Beyla 2.0.
If we want to keep backwards compatibility, maybe we could default it to prometheus
, and explain that it will only have effect if the prometheus
subsection is set.
But anyway that was just a suggestion. I'm fine with current implementation.
This PR adds the ability to exporter Beyla internal metrics for users that are using OpenTelemetry metrics.
Fixes #1211