Skip to content
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.

Sign up for GitHub

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

Fix prometheusexporter droping the OTEL resource labels #2899

Merged
merged 8 commits into from
Apr 21, 2021
Merged

Fix prometheusexporter droping the OTEL resource labels #2899

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0be56e9
fix: add resource_attributes_as_tags support prometheusexporter
krak3n Apr 7, 2021
3286b18
fix: prometheus exporter test cases
krak3n Apr 7, 2021
db5077d
fix: yaml indentation
krak3n Apr 7, 2021
97bbba7
chore: add test case to ensure resource labels are added to prometheu…
krak3n Apr 8, 2021
fa01a46
Merge branch 'main' into fix/prometheus-exporter-resource-labels
krak3n Apr 8, 2021
d7f1076
fix: missing test case for exporterhelper.NewMetricsExporter error
krak3n Apr 8, 2021
4179644
refactor: use existing resource_to_telemetry_conversion convention fr…
krak3n Apr 13, 2021
1d5a944
Merge branch 'main' into fix/prometheus-exporter-resource-labels
krak3n Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions exporter/prometheusexporter/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ The following settings can be optionally configured:
- `send_timestamps` (default = `false`): if true, sends the timestamp of the underlying
metric sample in the response.
- `metric_expiration` (default = `5m`): defines how long metrics are exposed without updates
- `resource_attributes_as_tag` (default = `false`): if set to true will transform all resource
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking into this exact same feature with the prometheusremotewriteexporter. Was just wondering - strictly speaking it is "resource attributes as labels"? I guess Prometheus uses the term "label" instead of "tag"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @bogdandrutu I'll switch it to a convention thats used over in the contrib repo so things are kept consistent, so this will change too

        resource_to_telemetry_conversion:
            enabled: true

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to be tags not tag

attributes into labels.

Example:

Expand All @@ -30,4 +32,5 @@ exporters:
"another label": spaced value
send_timestamps: true
metric_expiration: 180m
resource_attributes_as_tags: true
```
4 changes: 4 additions & 0 deletions exporter/prometheusexporter/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,8 @@ type Config struct {

// MetricExpiration defines how long metrics are kept without updates
MetricExpiration time.Duration `mapstructure:"metric_expiration"`

// ResourceAttributesAsTags, if sMet to true, will use the exporterhelper feature to
// transform all resource attributes into metric labels.
ResourceAttributesAsTags bool `mapstructure:"resource_attributes_as_tags"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In contrib where we use this, we have config like:

	exporterhelper.ResourceToTelemetrySettings `mapstructure:"resource_to_telemetry_conversion"`

so the config will be:

   resource_to_telemetry_conversion:
      enabled: true

If you don't like that name maybe embed that settings but use the name resource_attributes_as_tags? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah cool, it should be consistent with existing implementations so happy to switch it up to:

        resource_to_telemetry_conversion:
            enabled: true

}
29 changes: 28 additions & 1 deletion exporter/prometheusexporter/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,5 +52,32 @@ func createMetricsExporter(
) (component.MetricsExporter, error) {
pcfg := cfg.(*Config)

return newPrometheusExporter(pcfg, params.Logger)
prometheus, err := newPrometheusExporter(pcfg, params.Logger)
if err != nil {
return nil, err
}

exporter, err := exporterhelper.NewMetricsExporter(
cfg,
params.Logger,
prometheus.ConsumeMetrics,
exporterhelper.WithStart(prometheus.Start),
exporterhelper.WithShutdown(prometheus.Shutdown),
exporterhelper.WithResourceToTelemetryConversion(exporterhelper.ResourceToTelemetrySettings{
Enabled: pcfg.ResourceAttributesAsTags,
}),
)
if err != nil {
return nil, err
}

return &wrapMetricsExpoter{
MetricsExporter: exporter,
exporter: prometheus,
}, nil
}

type wrapMetricsExpoter struct {
component.MetricsExporter
exporter *prometheusExporter
}
4 changes: 2 additions & 2 deletions exporter/prometheusexporter/prometheus_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ func TestPrometheusExporter_endToEnd(t *testing.T) {
}

// Expired metrics should be removed during first scrape
exp.(*prometheusExporter).collector.accumulator.(*lastValueAccumulator).metricExpiration = 1 * time.Millisecond
exp.(*wrapMetricsExpoter).exporter.collector.accumulator.(*lastValueAccumulator).metricExpiration = 1 * time.Millisecond
time.Sleep(10 * time.Millisecond)

res, err := http.Get("http://localhost:7777/metrics")
Expand Down Expand Up @@ -238,7 +238,7 @@ func TestPrometheusExporter_endToEndWithTimestamps(t *testing.T) {
}

// Expired metrics should be removed during first scrape
exp.(*prometheusExporter).collector.accumulator.(*lastValueAccumulator).metricExpiration = 1 * time.Millisecond
exp.(*wrapMetricsExpoter).exporter.collector.accumulator.(*lastValueAccumulator).metricExpiration = 1 * time.Millisecond
time.Sleep(10 * time.Millisecond)

res, err := http.Get("http://localhost:7777/metrics")
Expand Down