feat(exporter-prometheus): add additional attributes option #5317
Conversation
|
I think the way to accomplish this would be to implement this specification: https://github.com/open-telemetry/opentelemetry-specification/blob/01066fe9112ce4dad4371418d88161221bf8db3e/specification/metrics/sdk_exporters/prometheus.md?plain=1#L60-L64 |
|
The way the specification suggests to handle this is more closely aligned with OTel concepts and would also solve #5073 |
marius-a-mueller
force-pushed
the
feature/add_additional_attributes_to_prometheus_exporter
branch
from
bdd99a6 to
bd68f6b
Compare
|
Okay, I implemented the feature as written in the specification. I just need a bit longer for the CLA authorization. |
marius-a-mueller
force-pushed
the
feature/add_additional_attributes_to_prometheus_exporter
branch
from
bd68f6b to
a36a818
Compare
|
And now I have the CLA authorization too. |
| @@ -43,6 +44,7 @@ export class PrometheusExporter extends MetricReader { | |||
| private readonly _server: Server; | |||
| private readonly _prefix?: string; | |||
| private readonly _appendTimestamp: boolean; | |||
| private readonly _withResourceConstantLabels?: string | RegExp; | |||
There was a problem hiding this comment.
since this is only used in the constructor, we don't need to keep this as a property, right?
| constructor( | ||
| prefix?: string, | ||
| appendTimestamp = false, | ||
| withResourceConstantLabels?: string | RegExp |
There was a problem hiding this comment.
Hmm, what's the reason to accept string in addition to RegExp? 🤔
| const filteredAttributes: Attributes = {}; | ||
| for (const [key, value] of Object.entries(attributes)) { | ||
| const sanitizedAttributeName = sanitizePrometheusMetricName(key); | ||
| if (sanitizedAttributeName.match(pattern)) { |
There was a problem hiding this comment.
I wonder if it would be better to use the resource attribute name before it's sanitized. @opentelemetry/semantic-conventions includes constants for well-known resource attributes, which can be re-used. This way around people have to know what the attribute would be named converted to Prometheus naming convention, which may introduce some friction.
There was a problem hiding this comment.
I have not checked what other language SIGs do, if they do the same as here then we can leave it as-is. Otherwise it may be better to follow what the majority of other SIGs are doing to be consistent across languages. :)
Which problem is this PR solving?
At the moment there is no way to set default/static attributes for all metrics that are being exported by the Prometheus exporter. This PR adds an option for adding resource attributes as metric attributes.
Fixes # (issue)
Short description of the changes
withResourceConstantLabelsoption toExporterConfigType of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
npm testChecklist: