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/prometheusremotewriteexporter] Deprecate and Remove export_created_metric feature. #35003

Open
ArthurSens opened this issue Sep 4, 2024 · 10 comments · Fixed by #36214

Comments

@ArthurSens
Copy link
Member

ArthurSens commented Sep 4, 2024

Component(s)

exporter/prometheusremotewrite

Describe the issue you're reporting

Although the intention with #17417 was good to be compliant with OpenMetrics, it's important to mention that OpenMetrics is an data exposition format, while Prometheus Remote-Write is a data transport protocol.

The original goal of StartTimeUnixNano and OM's _created metrics is to identify resets of monotonically increasing metrics.

Knowing that our exporter exposes StartTimeUnixNano as a separate metric, and that PRWv1 potentially splits metrics into multiple requests, there's no guarantee that _created and its related metric will be sent in the same request, which makes the usage of this feature super super difficult and inefficient. The current implementation in Prometheus when receiving this metric will just create another timeseries and will not use it to identify resets, making this feature in the collector pointless.

My suggestion here is that we remove this feature altogether. Let's keep in mind that this is a prwexporter and not a openmetrics exporter.

@ArthurSens ArthurSens added the needs triage New item requiring triage label Sep 4, 2024
Copy link
Contributor

github-actions bot commented Sep 4, 2024

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@dashpole
Copy link
Contributor

dashpole commented Sep 4, 2024

Since PRW v2.0 will have the created timestamp in the proto, this will become moot after we switch to that. Should we just leave it around until then? IIRC, it is disabled by default. I'm not sure if people are using it or not. We can add a warning or something to the documentation if you want to make sure people know it doesn't work with Prometheus' CT support

@dashpole dashpole removed the needs triage New item requiring triage label Sep 4, 2024
@ArthurSens
Copy link
Member Author

ArthurSens commented Sep 5, 2024

I'd rather remove it, if possible. For PRWv2 I think we'll always do this conversion, it shouldn't be opt-in/out 🤔. While in PRWv1 it just doesn't work at all, so there's no reason to enable it

@dashpole
Copy link
Contributor

dashpole commented Sep 5, 2024

cc @kovrus do you have any thoughts here?

Copy link
Contributor

github-actions bot commented Nov 5, 2024

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Nov 5, 2024
@ArthurSens
Copy link
Member Author

Sorry, this is still on my list. Just struggling to allocate time for it 😬

@pokgak
Copy link
Contributor

pokgak commented Nov 5, 2024

@ArthurSens here's the PR for deprecation: #36214

If I understand the guideline correctly, removing the config option itself have to be done separately in the next release right?

@ArthurSens
Copy link
Member Author

@ArthurSens here's the PR for deprecation: #36214

If I understand the guideline correctly, removing the config option itself have to be done separately in the next release right?

Yes, for complete removal it will take a few releases... but we can put the feature behind a feature gate and slowly decrease the stability stage until complete removal

@ArthurSens
Copy link
Member Author

Now that v0.114.0 has been released, we can switch the feature flag to StageBeta. Should we re-open this issue until the whole deprecation is finished? :)

@TylerHelmuth
Copy link
Member

Yes

@TylerHelmuth TylerHelmuth reopened this Nov 19, 2024
codeboten pushed a commit that referenced this issue Dec 3, 2024
#### Description

Change `exporter.prometheusremotewriteexporter.deprecateCreatedMetric`
feature gate from Alpha to Beta

#### Link to tracking issue

#35003

---------

Co-authored-by: David Ashpole <[email protected]>
shivanthzen pushed a commit to shivanthzen/opentelemetry-collector-contrib that referenced this issue Dec 5, 2024
…lemetry#36606)

#### Description

Change `exporter.prometheusremotewriteexporter.deprecateCreatedMetric`
feature gate from Alpha to Beta

#### Link to tracking issue

open-telemetry#35003

---------

Co-authored-by: David Ashpole <[email protected]>
ZenoCC-Peng pushed a commit to ZenoCC-Peng/opentelemetry-collector-contrib that referenced this issue Dec 6, 2024
…lemetry#36606)

#### Description

Change `exporter.prometheusremotewriteexporter.deprecateCreatedMetric`
feature gate from Alpha to Beta

#### Link to tracking issue

open-telemetry#35003

---------

Co-authored-by: David Ashpole <[email protected]>
RutvikS-crest pushed a commit to RutvikS-crest/opentelemetry-collector-contrib that referenced this issue Dec 9, 2024
…n-telemetry#36214)

#### Description

Deprecate export_created_metric config option

#### Link to tracking issue
Fixes open-telemetry#35003

---------

Co-authored-by: Arthur Silva Sens <[email protected]>
Co-authored-by: David Ashpole <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants