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] Update is_monotonic and aggregation_temporality accessor functions #10197

Merged
merged 4 commits into from
May 23, 2022

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented May 20, 2022

Description:
The access functions for metric's is_monotonic and aggregation_temporality fields were using the pmetric versions, but this didn't work in practice since the grammar doesn't know how to convert the incoming string to the exact pmetric equivalent.

Instead, I have updated the getters and setters to convert the pmetric type to an equivalent data type that the parser understands. This will allow the following scenarios to work:

Changing temporality: set(metric.aggregation_temporality, 1) where metric.aggregation_temporality == 0
Changing monotonicity: set(metric.is_monotonic, "true") where metric.is_monotonic == "false"

Testing:
Updated existing scenarios and added new test scenarios.

Documentation:
Updated readme.

@project-bot project-bot bot added this to In progress in Collector May 20, 2022
@TylerHelmuth
Copy link
Member Author

/cc @anuraaga

Collector automation moved this from In progress to Reviewer approved May 23, 2022
@TylerHelmuth
Copy link
Member Author

@open-telemetry/collector-contrib-maintainer please review. I'd like to get this in for the 0.52.0 release

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

LGTM.

One non-blocking question - since aggregation_temporality does not match the recently established naming conventions for functions in this processor, should we consider adding an alias and deprecating the original?

@djaglowski djaglowski merged commit ef9ef93 into open-telemetry:main May 23, 2022
@TylerHelmuth
Copy link
Member Author

One non-blocking question - since aggregation_temporality does not match the recently established naming conventions for functions in this processor, should we consider adding an alias and deprecating the original?

aggregation_temporality is the protobuf name of the field right? What convention is being broken?

@TylerHelmuth TylerHelmuth deleted the fix-metrics-accessors branch May 23, 2022 23:38
@djaglowski
Copy link
Member

Oops, crossed wires. I see that it's exactly as it should be. 👍

kentquirk pushed a commit to McSick/opentelemetry-collector-contrib that referenced this pull request Jun 14, 2022
… accessor functions (open-telemetry#10197)

* Update accessor functions

* Update readme

* Update changelog

* Update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Collector
  
Reviewer approved
Development

Successfully merging this pull request may close these issues.

None yet

4 participants