-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[pkg/ottl] Introduce profiles based e2e tests #40931
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
Changes from 5 commits
fccce03
e8ae5d3
526f8ed
ab00d80
f2e3720
26833b2
49eb460
0e00db8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| # Use this changelog template to create an entry for release notes. | ||
|
|
||
| # One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
| change_type: enhancement | ||
|
|
||
| # The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) | ||
| component: pkg/ottl | ||
|
|
||
| # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
| note: Add e2e test based on profiles | ||
|
|
||
| # Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. | ||
| issues: [40738] | ||
|
|
||
| # (Optional) One or more lines of additional information to render under the primary note. | ||
| # These lines will be padded with 2 spaces and then inserted directly into the document. | ||
| # Use pipe (|) for multiline entries. | ||
| subtext: | ||
|
|
||
| # If your change doesn't affect end users or the exported elements of any package, | ||
| # you should instead start your pull request title with [chore] or use the "Skip Changelog" label. | ||
| # Optional: The change log or logs in which this entry should be included. | ||
| # e.g. '[user]' or '[user, api]' | ||
| # Include 'user' if the change is relevant to end users. | ||
| # Include 'api' if there is a change to a library API. | ||
| # Default: '[user]' | ||
| change_logs: [user] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -348,11 +348,27 @@ func accessAttributesKey[K Context](key []ottl.Key[K]) ottl.StandardGetSetter[K] | |
| if err != nil { | ||
| return err | ||
| } | ||
| v := pcommon.NewValueEmpty() | ||
| v := getAttributeValue(tCtx, *newKey) | ||
| if err = ctxutil.SetIndexableValue[K](ctx, tCtx, v, val, key[1:]); err != nil { | ||
| return err | ||
| } | ||
| return pprofile.PutAttribute(tCtx.GetProfilesDictionary().AttributeTable(), tCtx.GetProfile(), *newKey, v) | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| func getAttributeValue[K ProfileContext](tCtx K, key string) pcommon.Value { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit confused with this fix, is overriding the existing keys at the profile dictionary expected here? considering it would indirectly change the value for all other references? Last time we discussed it duplicating keys was expected (#39681 (comment)), have it changed? If I'm not mistaken, that described scenario won't happen anymore with this fix, and the dictionary table item will be updated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good spot! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will also change the changelog to bug_fix and amend the description. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The validator could possibly be moved into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Converters should not change telemetry data, so it should be fine as it's (although we know they can sneakily do it by leveraging references). |
||
| // Find the index of the attribute in the profile's attribute indices | ||
| // and return the corresponding value from the attribute table. | ||
| table := tCtx.GetProfilesDictionary().AttributeTable() | ||
| indices := tCtx.GetProfile().AttributeIndices().AsRaw() | ||
|
|
||
| for _, tableIndex := range indices { | ||
| attr := table.At(int(tableIndex)) | ||
| if attr.Key() == key { | ||
| return attr.Value() | ||
| } | ||
| } | ||
|
|
||
| return pcommon.NewValueEmpty() | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No changelog needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but I think we should add a change log for the bug this PR is fixing.