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

[pkg/ottl] Add support for scaling values #33246

Merged
merged 43 commits into from
Jul 25, 2024
Merged

[pkg/ottl] Add support for scaling values #33246

merged 43 commits into from
Jul 25, 2024

Conversation

bacherfl
Copy link
Contributor

@bacherfl bacherfl commented May 28, 2024 •
edited
Loading

Description:

Adds a Scale function to the OTTL package. This function can be applied to int/double values, as well as metrics of the following types:

  • Sum
  • Gauge
  • Histogram

Link to tracking Issue: #16214

Testing: Added Unit and E2E tests in the OTTL package. Tested manually in a sample environment with the following example configuration:

receivers:
  otlp:
    protocols:
      grpc:
        endpoint: 0.0.0.0:4317

processors:
  transform:
    error_mode: ignore
    metric_statements:
      - context: metric
        statements:
          - set(data_points, Scale(data_points, 10.0))

exporters:
  debug:
    verbosity: detailed
  otlphttp:
    endpoint: "######"
    headers:
      Authorization: "#####"

service:
  pipelines:
    metrics:
      receivers: [otlp]
      exporters: [otlphttp, debug]
      processors: [transform]

Documentation: Added documentation in the README describing all functions in the ottl package

@bacherfl
[processor/transform] Add support for scaling values
b782416 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
revert change to e2e test
adcda48 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
rename helper function
4ebb38a 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented May 28, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@bacherfl bacherfl marked this pull request as ready for review May 29, 2024 07:53
@bacherfl bacherfl requested a review from a team May 29, 2024 07:53
@evan-bradley evan-bradley changed the title [processor/transform] Add support for scaling values [pkg/ottl] Add support for scaling values May 29, 2024
evan-bradley
evan-bradley reviewed May 29, 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 one on, it's looking a bit more involved than I thought at first glance.

pkg/ottl/ottlfuncs/func_scale.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_scale.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_scale.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/README.md Outdated Show resolved Hide resolved
@bacherfl
incorporate feedback from PR review
4084b53 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
Merge remote-tracking branch 'bacherfl/feat/16214/scale-metric-values…
35064e1 
…' into feat/16214/scale-metric-values
@bacherfl
add license comment
343ce4e 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
evan-bradley
evan-bradley reviewed Jun 4, 2024
View reviewed changes
pkg/ottl/ottlfuncs/func_scale.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/README.md Outdated Show resolved Hide resolved
@bacherfl
address PR review comments, add changelog entry
549bdc3 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
fix linting
fb04e21 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
adapt the function to only work with metrics
75a6d67 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
remove obsolete test
14bbdd8 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
evan-bradley
evan-bradley reviewed Jun 18, 2024
View reviewed changes
pkg/ottl/ottlfuncs/README.md Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_scale.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_scale.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_scale.go Outdated

type ScaleArguments[K any] struct {
Value ottl.GetSetter[K]
Multiplier float64
Copy link
Contributor

Choose a reason for hiding this comment

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

Like @TylerHelmuth mentioned, could we add an optional parameter to adjust the unit? I think it would be nice for usability to do all scaling in one statement.

bacherfl and others added 2 commits June 19, 2024 14:49
@bacherfl @evan-bradley
apply suggestions from PR review
0090fcc 
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
@bacherfl
move scale function to transformprocessor
d398d64 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@github-actions github-actions bot added the processor/transform Transform processor label Jun 19, 2024
@bacherfl
fix failing checks
e495e03 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
fix linting
c83e5ba 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
appease linter
66c5bd5 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
Copy link
Contributor Author

@TylerHelmuth @evan-bradley Thanks for the reviews- I have made the requested changes (moved the implementation to the transformprocessor and added an optional unit parameter)

@bacherfl
make unit an optional value
bf746e7 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
Merge branch 'main' into feat/16214/scale-metric-values
ffed4f1
@bacherfl
adapt unit tests to recent changes
719daca 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
evan-bradley
evan-bradley approved these changes Jul 1, 2024
View reviewed changes
.chloggen/add-scale-function.yaml Outdated Show resolved Hide resolved
TylerHelmuth
TylerHelmuth reviewed Jul 1, 2024
View reviewed changes
pkg/ottl/e2e/e2e_test.go Outdated Show resolved Hide resolved
processor/transformprocessor/internal/metrics/func_scale_test.go Show resolved Hide resolved
processor/transformprocessor/internal/metrics/func_scale.go Outdated
Comment on lines 55 to 61
if !args.Unit.IsEmpty() {
unit, err := args.Unit.Get().Get(ctx, tCtx)
if err != nil {
return nil, fmt.Errorf("could not get unit from ScaleArguments: %w", err)
}
metric.SetUnit(unit)
}
Copy link
Member

Choose a reason for hiding this comment

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

We should do this check first - if a unit is specified and we can't retrieve it we shouldnt scale the metric. Actually setting the unit can happen after the switch statement to ensure we only update the metric if we pass the error checks in the switch statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good suggestion, I have adapted the logic accordingly

bacherfl and others added 2 commits July 2, 2024 06:55
@bacherfl @evan-bradley
apply suggestion from code review
721c537 
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
@bacherfl
adapt to pr review
a6c39a1 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl
Copy link
Contributor Author

@TylerHelmuth can you please have another look?

bacherfl and others added 2 commits July 23, 2024 06:56
@bacherfl @TylerHelmuth
Apply suggestions from code review
876afc1 
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
@bacherfl
incorporate review comments
42d3140 
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@TylerHelmuth TylerHelmuth merged commit 1f88246 into open-telemetry:main Jul 25, 2024
156 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 25, 2024
@bacherfl bacherfl mentioned this pull request Aug 19, 2024
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TylerHelmuth TylerHelmuth TylerHelmuth approved these changes

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

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

@kentquirk kentquirk Awaiting requested review from kentquirk

Assignees

@bryan-aguilar bryan-aguilar

Labels
pkg/ottl processor/transform Transform processor
Projects
None yet
Milestone
v0.106.0
Development

Successfully merging this pull request may close these issues.
4 participants
@bacherfl @evan-bradley @TylerHelmuth @bryan-aguilar