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/metricstransformprocessor]: Support median aggregation type #33655

Merged

Conversation

odubajDT
Copy link
Contributor

@odubajDT odubajDT commented Jun 19, 2024

Description:

  • use aggregation business logic from interval/core
  • support median aggregation type
  • testing
  • docs

Link to tracking Issue: #16224

Depends on #33669

@github-actions github-actions bot added internal/core processor/metricstransform Metrics Transform processor labels Jun 19, 2024
@github-actions github-actions bot requested a review from dmitryax June 19, 2024 13:19
@odubajDT odubajDT force-pushed the aggregation-metricstransformprocessor branch 2 times, most recently from 4d0ee19 to 33c5372 Compare June 20, 2024 06:55
@odubajDT odubajDT changed the title [processor/metricstransformprocessor]: refactor aggregation business logic [processor/metricstransformprocessor]: Support median aggregation type Jun 20, 2024
@odubajDT odubajDT force-pushed the aggregation-metricstransformprocessor branch from 268ed54 to 8eabf28 Compare June 20, 2024 09:16
evan-bradley pushed a commit that referenced this pull request Jul 2, 2024
**Description:** <Describe what has changed.>
- duplicated and enhanced aggregation business logic (with median
function) for common usage in follow-up tickets
- tests

**Link to tracking Issue:** #16224 

**Follow-ups:**
-
#33655
-
#33334
-
#33423

---------

Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@odubajDT odubajDT force-pushed the aggregation-metricstransformprocessor branch 2 times, most recently from def6d9f to 451b941 Compare July 3, 2024 08:05
@odubajDT odubajDT marked this pull request as ready for review July 3, 2024 12:04
@odubajDT odubajDT requested a review from a team July 3, 2024 12:04
@odubajDT odubajDT force-pushed the aggregation-metricstransformprocessor branch 2 times, most recently from ac49460 to 88b7084 Compare July 10, 2024 07:39
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.

Looks good to me based on the fact that the tests have been minimally changed.

@evan-bradley
Copy link
Contributor

@dmitryax Could you take a look? This bridges what I believe is the final gap between the metrics transform processor and transform processor, and will let us confidently merge #33334.

I'll do a review after this to confirm, but I think once we know there is parity between the two, we can start pointing users to the transform processor as a meaningful replacement.

@odubajDT odubajDT force-pushed the aggregation-metricstransformprocessor branch 2 times, most recently from 9587143 to a97a4c2 Compare July 17, 2024 06:13
odubajDT and others added 2 commits July 18, 2024 08:22
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
…r_otlp.go

Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
@odubajDT odubajDT force-pushed the aggregation-metricstransformprocessor branch from a97a4c2 to e9a6f45 Compare July 18, 2024 06:23
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM

@dmitryax dmitryax merged commit 1821ecd into open-telemetry:main Jul 22, 2024
155 of 156 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 22, 2024
TylerHelmuth added a commit that referenced this pull request Oct 11, 2024
**Description:** <Describe what has changed.>
It is a valid use case to aggregate against an empty label set, which
will functionally clear all attributes. This behaviour was removed in
#33655, which simplified the check to `len() == 0`, which covers the
case of the label set being `nil` and having 0 elements as the same
scenario. However, these are not the same scenario and have different
meanings. This PR reintroduces the original behaviour, but in a more
efficient way by recognizing a label set with 0 elements and clearing
the attributes, which would be the logical conclusion after running the
filter anyway.

**Link to tracking Issue:** #34430

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

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

---------

Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this pull request Dec 17, 2024
…metry#35006)

**Description:** <Describe what has changed.>
It is a valid use case to aggregate against an empty label set, which
will functionally clear all attributes. This behaviour was removed in
open-telemetry#33655, which simplified the check to `len() == 0`, which covers the
case of the label set being `nil` and having 0 elements as the same
scenario. However, these are not the same scenario and have different
meanings. This PR reintroduces the original behaviour, but in a more
efficient way by recognizing a label set with 0 elements and clearing
the attributes, which would be the logical conclusion after running the
filter anyway.

**Link to tracking Issue:** open-telemetry#34430

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

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

---------

Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants