Allow user to pass in metrics namespace #5758
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5758 +/- ##
==========================================
- Coverage 91.78% 91.62% -0.16%
==========================================
Files 192 192
Lines 11394 11429 +35
==========================================
+ Hits 10458 10472 +14
- Misses 742 763 +21
Partials 194 194
Continue to review full report at Codecov.
|
CHANGELOG.md
Outdated
| @@ -14,6 +14,7 @@ | |||
|
|
|||
| - `ocb` now exits with an error if it fails to load the build configuration. (#5731) | |||
| - Deprecate `HTTPClientSettings.ToClientWithHost` (#5737) | |||
| - Add a new field `namespace` in `service.telemetry.metrics.namespace` to allow user to customize metric namespace (#5692) | |||
There was a problem hiding this comment.
| - Add a new field `namespace` in `service.telemetry.metrics.namespace` to allow user to customize metric namespace (#5692) | |
| - Add a new field `service.telemetry.metrics.namespace` to allow users to customize the collector's metric namespace (#5692) |
service/telemetry/config.go
Outdated
| @@ -102,4 +102,7 @@ type MetricsConfig struct { | |||
|
|
|||
| // Address is the [address]:port that metrics exposition should be bound to. | |||
| Address string `mapstructure:"address"` | |||
|
|
|||
| // NameSpace indicates the telemetry namespace the service will set. | |||
| NameSpace string `mapstructure:"namespace"` | |||
There was a problem hiding this comment.
I'd recommends going with Namespace instead, since it is a single word.
There was a problem hiding this comment.
sounds good. will update
service/telemetry.go
Outdated
| namespace := defaultNameSpace | ||
| if cfg.Metrics.NameSpace != "" { | ||
| namespace = cfg.Metrics.NameSpace | ||
| } | ||
| opts := prometheus.Options{ | ||
| Namespace: "otelcol", | ||
| Namespace: namespace, |
There was a problem hiding this comment.
This will only change the namespace when using OpenCensus metrics. For users that enable the useOtelForInternalMetricsfeatureGateID feature gate and use OTEL metrics, is the namespace not relevant? Or does initOpenTelemetry also need to be made aware of the namespace?
There was a problem hiding this comment.
there isn't a namespace set in the opentelemetry metrics(through useOtelForInternalMetricsfeatureGateID). I think there are still a lot of designs around that with the beta milestone so I will keep it as it is.
|
Issue description is bit confusing. Can you please reply to #5692 (comment) |
|
@dmitryax @TylerHelmuth any other comments or good to merge? |
service/telemetry/config.go
Outdated
| @@ -102,4 +102,7 @@ type MetricsConfig struct { | |||
|
|
|||
| // Address is the [address]:port that metrics exposition should be bound to. | |||
| Address string `mapstructure:"address"` | |||
|
|
|||
| // Namespace indicates the telemetry namespace the service will set. If nil, a default value is assigned. | |||
There was a problem hiding this comment.
The description is not very clear. As an otel collector user, I don't understand it without looking into the code. Please mention that it changes namespace of exposed OTel collector's metrics and that default namespace is otelcol. It maybe good to also show an example of how the metrics will look like if the namespace is changed for a generic metric like otelcol_process_uptime
There was a problem hiding this comment.
that is fair. Will add it to be more explicit.
There was a problem hiding this comment.
added in the new version
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description:
Adding a configuration that allows user to configure their metrics namespace
Link to tracking Issue:
#5692
Testing:
Added a unit test to verify namespace can be customized
Documentation:
Added godoc on the new
namespacefield