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

feat(parsers.avro): Allow union fields to be specified as tags #16272

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

fajpunk
Copy link
Contributor

@fajpunk fajpunk commented Dec 6, 2024

Summary

Flatten any fields specified in avro_tags before trying to convert their values to strings.

Without flattening, any union fields included in avro_tags do not actually get converted into Influx tags. This warning is logged:
2024-12-06T19:43:12Z W! [parsers.avro::file] Could not convert map[string:some_value] to string for tag "some_union_in_a_tag": type "map[string]interface {}" unsupported

Checklist

  • [x ] No AI generated code was used in this PR

Related issues

resolves #16271

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Dec 6, 2024

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@telegraf-tiger telegraf-tiger bot added fix pr to fix corresponding bug plugin/parser 1. Request for new parser plugins 2. Issues/PRs that are related to parser plugins labels Dec 6, 2024
@fajpunk
Copy link
Contributor Author

fajpunk commented Dec 6, 2024

!signed-cla

Copy link
Contributor

@athornton athornton left a comment

Choose a reason for hiding this comment

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

This looks OK to me. I don't know that I can actually review it since I don't have write access to influxdata:master, and we definitely need some InfluxData eyes on it, but I (or, I presume Dan) would be happy to talk anyone from InfluxData through what we're doing and why.

I think it's funny that it's our own team that eventually demanded an answer to Angelo's question

I have a question that comes from @afausti -- what should the default behavior be if you explicitly mark an incoming field as both a tag and a field? The current code "does both" which ... I don't know what that does in practice. The right answer is "don't do that," of course...but should I catch it when building the parser and warn and leave it a tag but not add it as a field?

from #13945

@athornton
Copy link
Contributor

Well, OK, not quite the same question, it's "what do we do if a field is not a simple type", but turning it into a string somehow is clearly the right answer and this seems like a pretty good way.

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Dec 6, 2024

@srebhan srebhan changed the title fix(parsers.avro): union fields can now be specified as tags feat(parsers.avro): Allow union fields to be specified as tags Dec 9, 2024
@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Dec 9, 2024
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Awesome contribution @fajpunk! Thanks you!

@athornton thanks for the review!

I relabeled this as a feature as this is what it is in my view. @DStrand1 what do you think?

@srebhan srebhan added ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. and removed fix pr to fix corresponding bug labels Dec 9, 2024
@DStrand1 DStrand1 merged commit bcea9a2 into influxdata:master Dec 10, 2024
29 checks passed
@github-actions github-actions bot added this to the v1.34.0 milestone Dec 10, 2024
justinwwhuang pushed a commit to justinwwhuang/telegraf_fork that referenced this pull request Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/parser 1. Request for new parser plugins 2. Issues/PRs that are related to parser plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avro union fields listed in avro_tags do not get converted to tags
4 participants