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

[processor/transform] Metric conversion functions use datapoint context instead of metric context #20773

Closed
Tracked by #28644
BinaryFissionGames opened this issue Apr 10, 2023 · 11 comments · Fixed by #29091
Labels
Contribfest enhancement New feature or request priority:p2 Medium processor/transform Transform processor

Comments

@BinaryFissionGames
Copy link
Contributor

Component(s)

processor/transform

Describe the issue you're reporting

The function convert_gauge_to_sum, convert_sum_to_gauge, convert_summary_sum_val_to_sum and convert_summary_count_val_to_sum all are using the datapoint context, but I'm not sure I understand why.

They all operate on the top-level "metric", so I think they should be using the metric context, since they should each run only once for each metric, and not once for each datapoint.

It seems like this was just retaining the old behavior before contexts were introduced, but I think these functions are actually better suited on the metric context.

@BinaryFissionGames BinaryFissionGames added the needs triage New item requiring triage label Apr 10, 2023
@github-actions github-actions bot added the processor/transform Transform processor label Apr 10, 2023
@github-actions
Copy link
Contributor

Pinging code owners:

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

@TylerHelmuth
Copy link
Member

At the time of their creation, which was during transformprocessor/OTTL infancy, we didn't have the concept of the metrics context, we only had spans, datapoints, and log records. You are correct that these are better suited for the metrics context.

@BinaryFissionGames
Copy link
Contributor Author

That makes sense. Are we OK with changing these to be on the metrics context (with a feature flag, since it would be a breaking change)?

I'd be willing to take that on, if you think so.

@TylerHelmuth
Copy link
Member

As long as we follow the feature gate deprecation process go for it.

@TylerHelmuth
Copy link
Member

@BinaryFissionGames I would suggest waiting until #18822 is merged to start your work.

@BinaryFissionGames
Copy link
Contributor Author

Got it, thanks for the heads up on that.

@atoulme atoulme added enhancement New feature or request and removed needs triage New item requiring triage labels Apr 12, 2023
@github-actions
Copy link
Contributor

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.

@evan-bradley
Copy link
Contributor

The plumbing to handle this was added in #24446. convert_summary_sum_val_to_sum and convert_summary_count_val_to_sum will likely be deprecated sometime after #22853 is closed, but I think convert_gauge_to_sum and convert_sum_to_gauge could be updated to take advantage of this.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2023

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 Oct 9, 2023
@evan-bradley evan-bradley added priority:p2 Medium and removed Stale labels Oct 9, 2023
@evan-bradley
Copy link
Contributor

@BinaryFissionGames are you still interested in working on this? It should be ready for implementation.

@BinaryFissionGames
Copy link
Contributor Author

Sorry for the late response @evan-bradley - I don't think I'll be getting around to this anytime soon, so if anyone else has any interest in taking it, they should totally go for it.

@TylerHelmuth TylerHelmuth added help wanted Extra attention is needed Contribfest and removed help wanted Extra attention is needed labels Oct 17, 2023
evan-bradley pushed a commit that referenced this issue Nov 20, 2023
…#29091)

**Description:** Allow running OTTL `convert_sum_to_gauge` and
`convert_gauge_to_sum` in metric context instead of datapoint when
`processor.transform.ConvertBetweenSumAndGaugeMetricContext` is enabled

closes #20773

<!--
**Testing:** <Describe what testing was performed and which tests were
added.>

**Documentation:** <Describe the documentation added.>
-->

This is the result of an effort at contribfest at Kubecon NA '23.

---------

Signed-off-by: Alex Boten <aboten@lightstep.com>
Co-authored-by: Faith Chikwekwe <faithchikwekwe01@gmail.com>
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Co-authored-by: Andreas Thaler <andreas.thaler01@sap.com>
Co-authored-by: Antoine Toulme <antoine@toulme.name>
Co-authored-by: Etienne Pelletier <etienne.pelletier@segment.com>
Co-authored-by: bryan-aguilar <46550959+bryan-aguilar@users.noreply.github.com>
Co-authored-by: Rajkumar Rangaraj <rajrang@microsoft.com>
Co-authored-by: Curtis Robert <crobert@splunk.com>
Co-authored-by: Alex Boten <aboten@lightstep.com>
Co-authored-by: Jacob Marble <jacobmarble@gmail.com>
Co-authored-by: Jon <jonnywamsley@gmail.com>
Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>
Co-authored-by: Antoine Toulme <antoine@lunar-ocean.com>
RoryCrispin pushed a commit to ClickHouse/opentelemetry-collector-contrib that referenced this issue Nov 24, 2023
…open-telemetry#29091)

**Description:** Allow running OTTL `convert_sum_to_gauge` and
`convert_gauge_to_sum` in metric context instead of datapoint when
`processor.transform.ConvertBetweenSumAndGaugeMetricContext` is enabled

closes open-telemetry#20773

<!--
**Testing:** <Describe what testing was performed and which tests were
added.>

**Documentation:** <Describe the documentation added.>
-->

This is the result of an effort at contribfest at Kubecon NA '23.

---------

Signed-off-by: Alex Boten <aboten@lightstep.com>
Co-authored-by: Faith Chikwekwe <faithchikwekwe01@gmail.com>
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Co-authored-by: Andreas Thaler <andreas.thaler01@sap.com>
Co-authored-by: Antoine Toulme <antoine@toulme.name>
Co-authored-by: Etienne Pelletier <etienne.pelletier@segment.com>
Co-authored-by: bryan-aguilar <46550959+bryan-aguilar@users.noreply.github.com>
Co-authored-by: Rajkumar Rangaraj <rajrang@microsoft.com>
Co-authored-by: Curtis Robert <crobert@splunk.com>
Co-authored-by: Alex Boten <aboten@lightstep.com>
Co-authored-by: Jacob Marble <jacobmarble@gmail.com>
Co-authored-by: Jon <jonnywamsley@gmail.com>
Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>
Co-authored-by: Antoine Toulme <antoine@lunar-ocean.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contribfest enhancement New feature or request priority:p2 Medium processor/transform Transform processor
Projects
None yet
4 participants