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] Add support for toggling scalar value types #11810

Closed
Mrod1598 opened this issue Jun 29, 2022 · 11 comments
Closed

[processor/transform] Add support for toggling scalar value types #11810

Mrod1598 opened this issue Jun 29, 2022 · 11 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers priority:p2 Medium processor/transform Transform processor

Comments

@Mrod1598
Copy link
Contributor

Is your feature request related to a problem? Please describe.
When collecting from prometheus, a value_type is assigned and sometimes incorrectly. Using this processor, it should be possible to toggle from double to int or the other way around in this processor.

Describe the solution you'd like
implementation of either the ability to use the set function to change it, or a new function to toggle it for a metric separate from the set function.

@TylerHelmuth TylerHelmuth added the processor/transform Transform processor label Jun 29, 2022
@TylerHelmuth
Copy link
Member

@Mrod1598 the metrics context allows accessing (Get and Set) of a scalar value using value_double and value_int. You can update a NumberDataPoint's value using set. I added this test locally in metrics/processor_test.go and it passed

{
	query: []string{`set(value_int, 2) where value_double == 1.0`},
	want: func(td pmetric.Metrics) {
		td.ResourceMetrics().At(0).ScopeMetrics().At(0).Metrics().At(0).Sum().DataPoints().At(0).SetIntVal(2)
	},
},

@TylerHelmuth TylerHelmuth added the priority:p3 Lowest label Jun 29, 2022
@Mrod1598
Copy link
Contributor Author

Got it thanks! is that documented somewhere that I missed?

@Mrod1598
Copy link
Contributor Author

wait actually isn't that changing the value of the actual data point and not toggling the type? my scenario is where the value is stored as a double but should be stored as an int.

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Jun 29, 2022

Got it thanks! is that documented somewhere that I missed?

I think this is an instance where the README's statement of "everything can be accessed using the OTLP protobuf names" isn't completely honest. To handle how pdata represents the protobuf's requirements of value for NumberDataPoint the TQL had to make up a accessor and value_double and value_int were chosen. There are some other scenarios like that that should be improved in the README. In fact the whole README is getting kinda messy. I plan to address it with #11751.

wait actually isn't that changing the value of the actual data point and not toggling the type? my scenario is where the value is stored as a double but should be stored as an int.

pdata (what the Transform Processor is beholden to) doesn't have a concept of "change the value type without changing the value". pdata allows you to check the value type, but not set it. It allows you to set an int value (which updates the value type) and set a double value (which updates the value type)

For your scenario I tried:

set(value_int, value_double)

Which doesn't work because the float64 returned by value_double could not be asserted as an int64 during value_int's type assertion where it looks for an int64. So I actually don't think the TQL can handle this scenario right now bc it doesn't know to cast.

@TylerHelmuth TylerHelmuth added enhancement New feature or request priority:p2 Medium and removed priority:p3 Lowest labels Jun 29, 2022
@TylerHelmuth
Copy link
Member

I think we need a new Factory function that lets you cast to a Int. then we could do

set(value_int, Int(value_double))

@TylerHelmuth TylerHelmuth added help wanted Extra attention is needed good first issue Good for newcomers labels Jun 30, 2022
@deepto98
Copy link

deepto98 commented Jul 4, 2022

Can I pick this up?

@TylerHelmuth
Copy link
Member

@deepto98 It's yours

@TylerHelmuth TylerHelmuth removed the help wanted Extra attention is needed label Jul 4, 2022
mcdoker18 added a commit to mcdoker18/opentelemetry-collector-contrib that referenced this issue Aug 20, 2022
bogdandrutu pushed a commit that referenced this issue Sep 7, 2022
)

* [pkg/telemetryquerylanguage] Added Int factory function to TQL (#11810)

* [pkg/telemetryquerylanguage] review fixes

* [pkg/telemetryquerylanguage] review fixes

* [pkg/telemetryquerylanguage] changelog
@TylerHelmuth
Copy link
Member

@mcdoker18 do you plan to update the transform processor with the new function?

@mcdoker18
Copy link
Contributor

@TylerHelmuth sorry for the delay, I was on vacation :)

@bogdandrutu
Copy link
Member

@TylerHelmuth qqs on this:

  1. Why not using the lower letters for this? int()
  2. Can the setter for the int_value be changed and do the cast in this case instead of asking user to do this?

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Sep 27, 2022

@bogdandrutu current naming guidelines are that Factory Functions be named with uppercase to differentiate from regular functions.

We could probably update setters to be smarter, checking the passed in value and seeing if it can be casted. That would be a more generic solution if it works.

But there might still be good reasons for a Factory Function like during comparison. I don't remember how many casting assumptions we make in there. Could also be usable when passing parameters to other functions.

The factory function is pretty flexible whereas the setter is only usable when the destination is involved with needing a type change.

mcdoker18 added a commit to mcdoker18/opentelemetry-collector-contrib that referenced this issue Sep 27, 2022
bogdandrutu pushed a commit that referenced this issue Sep 28, 2022
…14338)

[processor/transform] Added the Int function to the transform processor (#11810)
bogdandrutu referenced this issue in bogdandrutu/opentelemetry-collector-contrib Sep 29, 2022
…14338)

[processor/transform] Added the Int function to the transform processor (#11810)
bogdandrutu added a commit that referenced this issue Sep 29, 2022
…14482)

[processor/transform] Added Int function to the transform processor (#14338)

[processor/transform] Added the Int function to the transform processor (#11810)

Co-authored-by: Vitalii Solodilov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers priority:p2 Medium processor/transform Transform processor
Projects
None yet
Development

No branches or pull requests

5 participants