-
Notifications
You must be signed in to change notification settings - Fork 19
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
[exporter/prometheus] add with_resource_constant_labels #67
[exporter/prometheus] add with_resource_constant_labels #67
Conversation
c2b8dbb
to
2841eb8
Compare
examples/kitchen-sink.yaml
Outdated
@@ -125,6 +125,8 @@ meter_provider: | |||
without_units: false | |||
without_type_suffix: false | |||
without_scope_info: false | |||
# Configure Prometheus Exporter to add resource attributes as metrics attributes. | |||
with_resource_as_constant_labels: "service\\.[.]*" |
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.
In #64 I proposed using the view instrument name wildcard syntax, where ?
matches any single character, and *
matches any number of characters including none.
The argument being that this avoids having to answer questions about which flavor of regex is used, and whether we can actually accommodate that across languages.
The downside of a simpler wildcard syntax is that you likely need to switch this parameter from a string
type to an array of strings, since you can easily run into situations where you can't express the desired set in a single expression.
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.
Right, another downside is with a wildcard there isn't a mechanism for exclusions built-in
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.
What do you think of #71?
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.
The benefit of #71 is that we get ahead of what I think will be a recurring pattern of wanting to provide a way in file config to specify an allow list or disallow list with pattern matching. Regex comes with the complexity of specifying the version of regular expressions, and if a language doesn't support the specification, we'll have worse interoperability because regexes will need to be adjusted for an individual language's requirements. Glob pattern matching is straight forward to implement and gets most of there of what you want to do with regex.
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.
What are your thoughts on how one would use #71 to include everything except something that matches? I'm thinking specifically in the collector where someone may want to include all resource attributes except for maybe one of them.
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 like the simplicity of wildcards better, so long as we retain the ability to have an include or exclude match, then that would solve my use-case
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.
Various components in the collector support the following for include/exclude rules:
include:
- /var/log/pods/default_*/*/*.log
exclude: []
@jack-berg if you're ok with this, it can be added to #71
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.
Yeah that sounds great. Do you want to reflect that in this PR and merge? Then I can update #71 in a followup to reflect that syntax we use 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.
This is waiting on open-telemetry/opentelemetry-specification#3837
2841eb8
to
c92ff52
Compare
c92ff52
to
48526e5
Compare
This add support for the following from the spec: > A Prometheus Exporter MAY offer configuration to add > resource attributes as metric attributes. By default, > it MUST NOT add any resource attributes as metric > attributes. The configuration SHOULD allow the user > to select which resource attributes to copy (e.g. > include / exclude or regular expression based). Copied > Resource attributes MUST NOT be excluded from target_info. Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
41b8e1f
to
3a812cb
Compare
This add support for the following from the spec:
See /~https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk_exporters/prometheus.md