feat: add support for configuring scope info metric attributes for the Prometheus exporter and refactor conversion to Prometheus metrics#5123
Conversation
MikeGoldsmith
left a comment
There was a problem hiding this comment.
This looks like it's on the right path. A few things to note:
-
_SCOPE_ATTR_CONFLICT_NAMESis defined but never used — we should either use it to explicitly filter conflicting scope attributes or remove it. -
PR description looks copy-pasted from #5122 — says "Resource metrics as Metric attributes" but this is about scope info.
-
Merge conflict concern — this, #5122, #5118, and #5117 all modify
PrometheusMetricReaderand_translate_to_prometheus. This is going to conflict with them, we need to coordinate what should be done in order or we'll be bumping into each other with rebases. -
nit: the nesting depth in
_translate_to_prometheusis getting deep — might be worth extracting the per-metric logic into a helper in a follow-up.
Sorry, got a little sloppy with this PR. I originally used |
|
@MikeGoldsmith I ended up doing a pretty major refactor of the exporter logic based on your suggestion, so we will likely want to merge this PR first. |
MikeGoldsmith
left a comment
There was a problem hiding this comment.
Looks good, thanks @herin049. Need to update PR title and description.
ef6fede to
4a590f0
Compare
4a590f0 to
be41170
Compare
|
Hey, just a heads-up from the Prometheus SIG: please hold off on the merge here. There's on going discussion happening on the PR that stabilizes this part of the spec open-telemetry/opentelemetry-specification#5056 |
|
Moving to draft until the SIG confirms this is the correct path. |
Description
Adds support for configuring scope attributes for the Prometheus exporter in accordance to the Prometheus spec and refactor conversion to Prometheus metrics.
Fixes #5112
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Does This PR Require a Contrib Repo Change?
Checklist: