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.

Sign up for GitHub

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

Jump to bottom

[processor/transform] introduce aggregate_on_attributes function for metrics #33334

Merged
merged 6 commits into from
Jul 29, 2024
Merged

[processor/transform] introduce aggregate_on_attributes function for metrics #33334

merged 6 commits into from
Jul 29, 2024

Conversation

odubajDT
Copy link
Contributor

@odubajDT odubajDT commented Jun 3, 2024 •
edited
Loading

Link to tracking Issue: #16224

Changes:

  • implemented aggregate_on_attributes function (supporting sum/min/max/mean/median) for Sum, Gauge, Histogram, ExponentialHistogram
  • tests
  • documentation

Depends on #33669

@github-actions github-actions bot added the processor/transform Transform processor label Jun 3, 2024
@odubajDT odubajDT changed the title [processor/transform] introduce aggregate function for metrics [WIP] [processor/transform] introduce aggregate functions for metrics [WIP] Jun 4, 2024
@github-actions github-actions bot added the processor/metricstransform Metrics Transform processor label Jun 6, 2024
@github-actions github-actions bot requested a review from dmitryax June 6, 2024 10:55
@odubajDT odubajDT changed the title [processor/transform] introduce aggregate functions for metrics [WIP] [processor/transform] introduce aggregate functions for metrics Jun 6, 2024
@odubajDT odubajDT force-pushed the feat/16224/aggregate branch 2 times, most recently from 9156fe4 to 2eb79ed Compare June 6, 2024 11:31
@odubajDT odubajDT marked this pull request as ready for review June 6, 2024 12:00
@odubajDT odubajDT requested a review from a team June 6, 2024 12:00
evan-bradley
evan-bradley reviewed Jun 6, 2024
View reviewed changes
processor/transformprocessor/README.md Outdated Show resolved Hide resolved
processor/transformprocessor/README.md Outdated Show resolved Hide resolved
processor/transformprocessor/README.md Outdated Show resolved Hide resolved
evan-bradley
evan-bradley reviewed Jun 7, 2024
View reviewed changes
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on. I'm still working through it, just a few comments so far.

.chloggen/feat_16224_aggregate.yaml Outdated Show resolved Hide resolved
processor/metricstransformprocessor/README.md Outdated Show resolved Hide resolved
processor/metricstransformprocessor/README.md Outdated Show resolved Hide resolved
processor/transformprocessor/internal/metrics/func_agregate_label_value_metrics.go Outdated Show resolved Hide resolved
processor/transformprocessor/internal/metrics/func_agregate_label_metrics.go Outdated Show resolved Hide resolved
processor/transformprocessor/internal/metrics/func_agregate_label_metrics.go Outdated Show resolved Hide resolved
@odubajDT odubajDT mentioned this pull request Jun 7, 2024
Merged
@odubajDT odubajDT changed the title [processor/transform] introduce aggregate functions for metrics [processor/transform] introduce aggregate_label function for metrics Jun 7, 2024
@odubajDT odubajDT changed the title [processor/transform] introduce aggregate_label function for metrics [processor/transform] introduce aggregate_on_attributes function for metrics Jun 11, 2024
evan-bradley
evan-bradley reviewed Jun 14, 2024
View reviewed changes
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

At a high level this looks good to me, I only have one minor note on the documentation. The functionality here is complex, so I will do a deeper dive tomorrow on the aggregation itself.

processor/transformprocessor/README.md Show resolved Hide resolved
@evan-bradley
Copy link
Contributor

@TylerHelmuth I'll comb over the aggregation logic in-depth, but do you agree with the overall design so far?

TylerHelmuth
TylerHelmuth reviewed Jun 14, 2024
View reviewed changes
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

It would be very cool if metricstransformprocessor and transformprocessor could share a library for all the pdata manipulation parts of the business logic. That would make me feel more confident that the 2 processors are doing equivalent work.

@odubajDT
Copy link
Contributor Author

odubajDT commented Jun 17, 2024 •
edited
Loading

It would be very cool if metricstransformprocessor and transformprocessor could share a library for all the pdata manipulation parts of the business logic. That would make me feel more confident that the 2 processors are doing equivalent work.

Thanks for the reply, where would you prefer to place such a library? Since the logic is currently present in two separate packages. Would you prefer creating a new module for it?

Thank you in advance!

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Jun 17, 2024 •
edited
Loading

@odubajDT somewhere in internal/coreinternals would be good. I think the right approach would be to duplicate what you need and hook up the metricstransformprocessor in 1 PR, and then in this PR use that new code. That will keep things smaller.

Also let me know if you think that the way the metricstransformprocessor is setup will not allow this idea. I am hoping the fact that both processors use pdata makes it possible to share business logic.

@odubajDT odubajDT marked this pull request as draft June 20, 2024 12:07
@odubajDT odubajDT mentioned this pull request Jun 20, 2024
Merged
odubajDT and others added 6 commits July 29, 2024 13:29
@odubajDT
[processor/transform] introduce aggregate_on_attributes function for …
1671029 
…metrics

Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@odubajDT
minor fix
698a1a3 
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@odubajDT
polish
a96118b 
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@odubajDT
adapt readme
f680107 
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@odubajDT @evan-bradley
Update processor/transformprocessor/README.md
11cac53 
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
@odubajDT
adapt README
45a34ec 
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
TylerHelmuth
TylerHelmuth approved these changes Jul 29, 2024
View reviewed changes
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

@evan-bradley the readme updates look correct to me based on my understanding of the metrictransformprocessors functionality, please check

@evan-bradley
Copy link
Contributor

The new readme description is correct based on my understanding as well. Thanks for catching that. I may do some wordsmithing in the future since it's a slightly complex process, but I think what we have is sufficient for now.

@evan-bradley evan-bradley merged commit 1446a03 into open-telemetry:main Jul 29, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 29, 2024
@odubajDT odubajDT mentioned this pull request Aug 26, 2024
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evan-bradley evan-bradley evan-bradley approved these changes

@TylerHelmuth TylerHelmuth TylerHelmuth approved these changes

@bogdandrutu bogdandrutu Awaiting requested review from bogdandrutu bogdandrutu is a code owner

@kentquirk kentquirk Awaiting requested review from kentquirk

@dmitryax dmitryax Awaiting requested review from dmitryax

@djaglowski djaglowski Awaiting requested review from djaglowski

@MovieStoreGuy MovieStoreGuy Awaiting requested review from MovieStoreGuy

@andrzej-stencel andrzej-stencel Awaiting requested review from andrzej-stencel

@crobert-1 crobert-1 Awaiting requested review from crobert-1

@dashpole dashpole Awaiting requested review from dashpole

@atoulme atoulme Awaiting requested review from atoulme

@jmacd jmacd Awaiting requested review from jmacd

@codeboten codeboten Awaiting requested review from codeboten

@fatsheep9146 fatsheep9146 Awaiting requested review from fatsheep9146

@jpkrohling jpkrohling Awaiting requested review from jpkrohling

@yurishkuro yurishkuro Awaiting requested review from yurishkuro

@mx-psi mx-psi Awaiting requested review from mx-psi

@tigrannajaryan tigrannajaryan Awaiting requested review from tigrannajaryan

@mwear mwear Awaiting requested review from mwear

@jsuereth jsuereth Awaiting requested review from jsuereth

@songy23 songy23 Awaiting requested review from songy23

@bryan-aguilar bryan-aguilar Awaiting requested review from bryan-aguilar

Assignees

@atoulme atoulme

@TylerHelmuth TylerHelmuth

Labels
cmd/githubgen cmd/opampsupervisor cmd/otelcontribcol otelcontribcol command cmd/oteltestbedcol cmd/telemetrygen telemetrygen command confmap/provider/s3provider confmap/provider/secretsmanagerprovider connector/count connector/datadog connector/exceptions connector/failover connector/grafanacloud connector/otlpjson connector/roundrobin connector/routing connector/servicegraph connector/spanmetrics connector/sum exporter/alertmanager exporter/alibabacloudlogservice Alibaba components exporter/awscloudwatchlogs awscloudwatchlogs exporter exporter/awsemf awsemf exporter exporter/awskinesis exporter/awss3 exporter/awsxray exporter/azuredataexplorer exporter/azuremonitor exporter/carbon exporter/cassandra exporter/clickhouse exporter/coralogix exporter/datadog Datadog components exporter/dataset exporter/elasticsearch internal/core processor/metricstransform Metrics Transform processor processor/transform Transform processor
Projects
None yet
Milestone
v0.106.0
Development

Successfully merging this pull request may close these issues.
4 participants
@odubajDT @evan-bradley @TylerHelmuth @atoulme