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.

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/prometheusremotewrite] deprecate export_created_metric #36214

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

pokgak
Copy link

@pokgak pokgak commented Nov 5, 2024

Description

Deprecate export_created_metric config option

Link to tracking issue

Fixes #35003

Copy link

linux-foundation-easycla bot commented Nov 5, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@pokgak pokgak force-pushed the exporter/prometheusremotewriteexporter/deprecate-created-metric branch from 846159b to 2d62181 Compare November 5, 2024 15:17
@pokgak pokgak marked this pull request as ready for review November 5, 2024 15:18
@pokgak pokgak requested a review from a team as a code owner November 5, 2024 15:18
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

Thank you @pokgak!

Not sure if this is documented well, but when deprecating we can put the functionality behind a feature gate that is enabled by default. One example of feature gate can be seen here. In the example though the stability is StageAlpha (disabled by default) and we want it to considered StageBeta so it's enabled by default.

In version 0.114.0 we'll finally change it to Alpha and in 0.115.0 finally remove the option.

.chloggen/35003-deprecate-export_created_metric.yaml Outdated Show resolved Hide resolved
exporter/prometheusremotewriteexporter/config.go Outdated Show resolved Hide resolved
exporter/prometheusremotewriteexporter/README.md Outdated Show resolved Hide resolved
@pokgak
Copy link
Author

pokgak commented Nov 7, 2024

@ArthurSens correct me if I'm wrong but isn't the feature already disabled by default so we should be using StageAlpha instead?

if cfg.CreatedMetric == nil {
cfg.CreatedMetric = &CreatedMetric{
Enabled: false,
}
}

I pushed a commit using StageBeta but note that this will change the default value of the export_created_metric.enabled to true.

Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

@ArthurSens correct me if I'm wrong but isn't the feature already disabled by default so we should be using StageAlpha instead?

if cfg.CreatedMetric == nil {
cfg.CreatedMetric = &CreatedMetric{
Enabled: false,
}
}

I pushed a commit using StageBeta but note that this will change the default value of the export_created_metric.enabled to true.

Yes you're correct, I believe I wasn't clear with my previous comment, let me re-phrase it.

What we want to do is add yet another check when we're generating the created metric.

if settings.ExportCreatedMetric && startTimestamp != 0 {
labels := createLabels(baseName+createdSuffix, baseLabels)
c.addTimeSeriesIfNeeded(labels, startTimestamp, pt.Timestamp())
}

Here we'd switch to:

 if settings.ExportCreatedMetric && startTimestamp != 0 && exportCreatedMetricGate.IsEnabled() { 
 	labels := createLabels(baseName+createdSuffix, baseLabels) 
 	c.addTimeSeriesIfNeeded(labels, startTimestamp, pt.Timestamp()) 
 } 

And then we'll have to update one test to make sure the feature-gate works as expected:

Add a new boolean to the test case struct, and in the the Run you can add:

oldValue := exportCreatedMetricGate.IsEnabled()
testutil.SetFeatureGateForTest(t, exportCreatedMetricGate, tt.isGateEnabled)
defer testutil.SetFeatureGateForTest(t, exportCreatedMetricGate, oldValue)

@@ -104,7 +114,7 @@ func (cfg *Config) Validate() error {
}
if cfg.CreatedMetric == nil {
cfg.CreatedMetric = &CreatedMetric{
Enabled: false,
Enabled: exportCreatedMetricGate.IsEnabled(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Enabled: exportCreatedMetricGate.IsEnabled(),
Enabled: false,

Let's go back to false!

Comment on lines +90 to +93
"exporter.prometheusremotewriteexporter.exportCreatedMetric",
featuregate.StageBeta,
featuregate.WithRegisterDescription("Set the default value of the export_created_metric feature."),
featuregate.WithRegisterReferenceURL("https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/35003"),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"exporter.prometheusremotewriteexporter.exportCreatedMetric",
featuregate.StageBeta,
featuregate.WithRegisterDescription("Set the default value of the export_created_metric feature."),
featuregate.WithRegisterReferenceURL("https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/35003"),
"exporter.prometheusremotewriteexporter.deprecateCreatedMetric",
featuregate.StageBeta,
featuregate.WithRegisterDescription("Feature gate used to control the deprecation of created metrics."),
featuregate.WithRegisterReferenceURL("https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/35003"),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[exporter/prometheusremotewriteexporter] Deprecate and Remove export_created_metric feature.
3 participants