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/attributesprocessor] Add a convert action #7930

Merged
merged 28 commits into from
Mar 16, 2022

Conversation

hughsimpson
Copy link
Contributor

@hughsimpson hughsimpson commented Feb 15, 2022

Description:
The attributes processor doesn't currently have support for type conversions. This can be necessary for 3rd parties that expect fields to be of a certain type (e.g. http.status_code might be received as a string but required to be an int by a service provider). This pr adds a convert action to the attributesprocessor, which would look like:

processors:
  attributes/convert:
      - key: http.status_code
        action: convert
        converted_type: int

Only very naive conversions have been included for now. There's no obvious behaviour for converting non-numeric types to numeric types (with the exception of numeric strings). For example, should [ 2 ] be 1 (length of array) or 2 (only value of singleton array)? Given that, all non-obvious values are converted to the zero value left unamended for now

Testing:
Tests added to the attribute processor for config and behaviour

Documentation:
Some copy has been added to the attributesprocessor readme to cover the new action type

@hughsimpson hughsimpson requested a review from a team February 15, 2022 09:38
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 15, 2022

CLA Signed

The committers are authorized under a signed CLA.

@pmm-sumo
Copy link
Contributor

pmm-sumo commented Feb 17, 2022

Thank you for the contribution @hughsimpson!

On the terminology part - I think that coerce is used in the context of an implicit action and in this case it's an explicit one, so perhaps casting or conversion would be more accurate?

@pmm-sumo
Copy link
Contributor

@bogdandrutu @anuraaga @jpkrohling how does it relate to transform processor (as I understand we eventually want to replace attributes and resource processors with it)? Should we add such capability here and later provide similar function there?

@hughsimpson hughsimpson changed the title Add a coerce action to the attributesprocessor module Add a convert action to the attributesprocessor module Feb 17, 2022
@pmm-sumo
Copy link
Contributor

@hughsimpson did another run and added few comments. Also, seems some checks are failing

@hughsimpson
Copy link
Contributor Author

hughsimpson commented Mar 12, 2022

Pushed a merge because the bot said this issue was stale... 🤷 Any chance of a review from @dashpole or @dmitryax at some point? Is there something I've missed? I don't mind tidying the commit history or whatever...

Edit: oh, well I see that the release notes need updating, at least. I'm many releases behind on that front now. Will push

@github-actions github-actions bot removed the Stale label Mar 13, 2022
@@ -87,8 +88,14 @@ func generateMetricData(resourceName string, attrs map[string]pdata.AttributeVal
pdata.NewAttributeMapFromMap(attrs).CopyTo(dps.At(i).Attributes())
dps.At(i).Attributes().Sort()
}
default:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tbqh this entire switch block is redundant but I'm leaving it in for now to reduce churn on the pr

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure why it is needed. Also, removing this block and uncommenting the test cases where you found issues leads to green tests :)

Copy link
Contributor Author

@hughsimpson hughsimpson Mar 13, 2022

Choose a reason for hiding this comment

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

That's because the tests didn't previously actually test anything. If you changed the expected attributes randomly they still passed (I noticed this because I had a typo in one of my tests but it still passed). This is because generateMetricData always returned a single None type metric with no attributes, so both the input and output generated the same empty data. If you would prefer, I could leave the tests as they are, but that felt kinda negligent to me

@pmm-sumo pmm-sumo dismissed their stale review March 13, 2022 18:12

Changes which require some fixes

@hughsimpson
Copy link
Contributor Author

Since there's now a ticket and pr to track the issues with the metrics attributesprocessor, I've reverted the test fixes in the hope that it can facilitate the process of this pr getting merged

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.

Overall LGTM, just couple nits

internal/coreinternal/attraction/type_converter.go Outdated Show resolved Hide resolved
processor/attributesprocessor/README.md Outdated Show resolved Hide resolved
@jpkrohling
Copy link
Member

Once @dmitryax approves, I'll merge this one.

@hughsimpson
Copy link
Contributor Author

@dmitryax please tell me this can get over the line now 😅

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. just couple of nits

internal/coreinternal/attraction/type_converter.go Outdated Show resolved Hide resolved
internal/coreinternal/attraction/type_converter.go Outdated Show resolved Hide resolved
@hughsimpson
Copy link
Contributor Author

@jpkrohling Think everything has been addressed now 🙏

@jpkrohling jpkrohling merged commit 1766aa2 into open-telemetry:main Mar 16, 2022
@hughsimpson hughsimpson deleted the add_coerce_action branch March 16, 2022 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants