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 metrics data model #9719

Merged

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented May 3, 2022

Description:
Adds the metrics data model to the transform process. Wiring up the processor to the metrics pipeline will be a different PR. Similar to #9271, logic has been stolen from traces and logs. Similar logic will be broken out later. Within metrics.go itself there is a lot of duplicate logic (especially with the switch statement). I am hoping generics can help with this in the future.

Link to tracking Issue:
#8252

Testing:

Documentation:

@TylerHelmuth
Copy link
Member Author

TylerHelmuth commented May 3, 2022

Open Questions:
1. How to handle accessing individual items within exemplars and quantile_values.
2. How to access DataType fields such as is_monotonic and aggregation_temporality

@bogdandrutu
Copy link
Member

/cc @anuraaga

@TylerHelmuth
Copy link
Member Author

  1. How to access DataType fields such as is_monotonic and aggregation_temporality

Handled via the descriptor. can access via descriptor.metric_aggregation_temporality and descriptor.metric_is_monotonic

@TylerHelmuth
Copy link
Member Author

TylerHelmuth commented May 5, 2022

  1. How to handle accessing individual items within exemplars and quantile_values.

I think we'll have to handle exemplars and quartile_values either via a new transformContext so that they can have their own newPathGetSetter or only ever as a list, and functions that want to mess with individual items will need to receive the whole list and then make decision. For this PR, I'll be opting on the latter, which is also the existing pattern for Events and Links.

@TylerHelmuth
Copy link
Member Author

Added tests for NumberDataPoint, HistogramDataPoint, ExponentialHistogramDataPoint, SummaryDataPoint, and Descriptor. I did not add tests for Resource or InstrumentationScope since the code is the same as traces and logs, which have tests, and we are planning to refactor both into a single place.

@TylerHelmuth TylerHelmuth marked this pull request as ready for review May 5, 2022 22:53
@TylerHelmuth TylerHelmuth requested a review from a team May 5, 2022 22:53
@TylerHelmuth
Copy link
Member Author

@anuraaga ready for official review

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Thanks! Just suggestion on the new method on TransformContext

processor/transformprocessor/internal/common/expression.go Outdated Show resolved Hide resolved
processor/transformprocessor/internal/metrics/metrics.go Outdated Show resolved Hide resolved
@TylerHelmuth
Copy link
Member Author

@open-telemetry/collector-contrib-approvers please review.

@dehaansa
Copy link
Contributor

Wired this up into a full processor locally to test out a use case we're looking for (resource attribute copied down to metric attribute) and it worked perfectly. 👍

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

@djaglowski
Copy link
Member

@TylerHelmuth should this have a changelog entry?

@TylerHelmuth
Copy link
Member Author

@TylerHelmuth should this have a changelog entry?

I didn't add one bc #9271 didn't have one. A changelog entry was added when logs were wired up (#9368)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants