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 ParseCSV converter #31081

Merged
merged 34 commits into from
Feb 16, 2024
Merged

[pkg/ottl]: Add ParseCSV converter #31081

merged 34 commits into from
Feb 16, 2024

Conversation

BinaryFissionGames
Copy link
Contributor

@BinaryFissionGames BinaryFissionGames commented Feb 6, 2024 •
edited
Loading

Description:

  • Adds a new ParseCSV converter that can parse CSV row strings.

Link to tracking Issue: Closes #30921

Testing:

  • Unit tests
  • Manually tested the examples with a local build of the collector

Documentation:

  • Adds documentation for using the ParseCSV converter.

@BinaryFissionGames BinaryFissionGames marked this pull request as ready for review February 6, 2024 18:50
@BinaryFissionGames BinaryFissionGames requested a review from a team February 6, 2024 18:50
djaglowski
djaglowski reviewed Feb 7, 2024
View reviewed changes
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

I'm not familiar enough with ottl code standards to approve this but it looks good to me from the perspective of matching csv parsing functionality in pkg/stanza.

evan-bradley
evan-bradley reviewed Feb 9, 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 contributing this. Overall looks good to me, just a few minor questions/notes.

pkg/ottl/ottlfuncs/func_parse_csv.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_parse_csv_test.go Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_parse_csv.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/README.md Outdated Show resolved Hide resolved
@BinaryFissionGames BinaryFissionGames force-pushed the feat/ottl/csv-parse-function branch from 39912b0 to 4c92b71 Compare February 12, 2024 16:01
evan-bradley
evan-bradley reviewed Feb 13, 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.

Overall looks good to me. If possible it would be nice to share parsing functionality between OTTL and Stanza, but if that's going to pose a significant challenge we can tackle it in a follow-up.

pkg/ottl/ottlfuncs/func_parse_csv.go Outdated Show resolved Hide resolved
@BinaryFissionGames
Copy link
Contributor Author

@evan-bradley @TylerHelmuth I've factored out the duplicate code between stanza and ottl into coreinternals, should be good for another look!

djaglowski
djaglowski reviewed Feb 14, 2024
View reviewed changes
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

LGTM but please add at least me as owner on a new line in .github/CODEOWNERS.

@BinaryFissionGames
Copy link
Contributor Author

LGTM but please add at least me as owner on a new line in .github/CODEOWNERS.

I added a new line for parseutils and put you as codeowner, it's not clear to me if that's OK to do (I know this repo has special tooling, and most lines in the codeowners file are modules and not packages), so let me know if I should be doing something different with that.

@TylerHelmuth
Copy link
Member

@djaglowski we are already code owners of internal/coreinternal, is that enough? Manual edits of the codeowner's file will get overridden.

@djaglowski
Copy link
Member

@djaglowski we are already code owners of internal/coreinternal, is that enough? Manual edits of the codeowner's file will get overridden.

I guess it's enough though I am particularly invested in ensuring stability of this code. I forgot it's a generated file at this point anyways so we can just leave it alone.

@TylerHelmuth
Copy link
Member

@djaglowski maybe a future reorganization from coreinternal (I'm not really sure what that means anyways) to something like internal/parsers?

@dpaasman00 dpaasman00 mentioned this pull request Feb 15, 2024
Merged
TylerHelmuth
TylerHelmuth reviewed Feb 15, 2024
View reviewed changes
pkg/ottl/ottlfuncs/func_parse_csv.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_parse_csv.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_parse_csv.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_parse_csv.go Outdated Show resolved Hide resolved
@BinaryFissionGames BinaryFissionGames force-pushed the feat/ottl/csv-parse-function branch from 1d89b65 to 481f577 Compare February 16, 2024 13:56
@BinaryFissionGames
Copy link
Contributor Author

@evan-bradley Rebased and checks are passing now!

@evan-bradley evan-bradley merged commit a852c86 into open-telemetry:main Feb 16, 2024
142 checks passed
@github-actions github-actions bot added this to the next release milestone Feb 16, 2024
@BinaryFissionGames BinaryFissionGames mentioned this pull request Feb 16, 2024
Merged
djaglowski pushed a commit that referenced this pull request Feb 16, 2024
@BinaryFissionGames
[chore][pkg/stanza]: Use shared parser code for CSV parsing (#31302)
5f81014 
**Description:** <Describe what has changed.>
* Follow up to #31081 to refactor using shared code between Stanza and
OTTL for parsing CSV.

**Testing:**
* Existing unit tests cover this refactor
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
@BinaryFissionGames @XinRanZhAWS
[pkg/ottl]: Add ParseCSV converter (open-telemetry#31081)
4386267 
**Description:**
* Adds a new ParseCSV converter that can parse CSV row strings.

**Link to tracking Issue:** Closes open-telemetry#30921

**Testing:**
* Unit tests
* Manually tested the examples with a local build of the collector

**Documentation:**
* Adds documentation for using the ParseCSV converter.
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
@BinaryFissionGames @XinRanZhAWS
[chore][pkg/stanza]: Use shared parser code for CSV parsing (open-tel…
539f75a 
…emetry#31302)

**Description:** <Describe what has changed.>
* Follow up to open-telemetry#31081 to refactor using shared code between Stanza and
OTTL for parsing CSV.

**Testing:**
* Existing unit tests cover this refactor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djaglowski djaglowski djaglowski left review comments

@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

@evan-bradley evan-bradley

Labels
internal/core pkg/ottl pkg/stanza
Projects
None yet
Milestone
v0.95.0
Development

Successfully merging this pull request may close these issues.

[pkg/ottl]: Add functions for parsing CSV
4 participants
@BinaryFissionGames @TylerHelmuth @djaglowski @evan-bradley