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

[pkg/ottl] Add ParseJSON factory function #16444

Merged
merged 17 commits into from
Nov 29, 2022

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented Nov 22, 2022

Description:
Adds a new factory function that can parse json strings into pcommon.Map types.

Link to tracking Issue:
Related to #9410
Related to #14938

Testing:
Unit tests

Documentation:
Added new function doc

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

I like the syntax, would like @djaglowski opinion on this.

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 think the syntax looks really nice for json, but I'm concerned it may not extend to other formats.

Comparing to the parsers in pkg/stanza, I think nearly all would need at least one additional parameter.

  • regex requires a regular expression
  • csv requires headers
  • keyvalue requires delimiter(s)
  • syslog requires a protocol (arguably these could enumerate as separate formats, but there are other options for each format as well)

Of course we could have separate functions for these cases, but if that's necessary then I'm wondering how generalizable this signature will be. One other parser from pkg/stanza that could work with this signature is uri - so there are at least two. Do you have any others in mind that would use this function signature?

@bogdandrutu
Copy link
Member

@djaglowski what about we have a variant argument at the end?

@TylerHelmuth
Copy link
Member Author

@djaglowski thats good perspective. I don't love the idea of trying to make a generic parser and then having to have specific parsers anyways because they require more input parameters.

I think the formats that would fit this signature would be json, uri, yaml, and toml so we could get at least 4 out of this type of function.

From an end user perspective, though, I think it is better if we are consistent with 1 strategy; I don't want users to have to know that some types of parsing are lumped into this function and others are their own functions.

If there is a common characteristic that connects json, uri, yaml, and toml that can be included in the name of this function, then I'd feel better about combining them. If csv is a specific structure that warrents a ParseCSVIntoMap, then maybe json, uri, yaml, and toml are all grouped by something like ParseStructureIntoMap?

@TylerHelmuth
Copy link
Member Author

TylerHelmuth commented Nov 23, 2022

@bogdandrutu we removed variadic support when support for explicit lists was added which I still support. I'm in favor of the language being explicit both in the function names and their parameters and variadic support breaks that.

@github-actions github-actions bot requested a review from bogdandrutu November 23, 2022 16:23
@TylerHelmuth
Copy link
Member Author

@kentquirk @evan-bradley what do you think?

@kentquirk
Copy link
Member

I don't see a plausible, nameable map format that someone will realistically stick into a single field other than JSON, which kind of occurs naturally, especially in JS environments. Attaching YAML or TOML is so fraught with danger, that doesn't seem plausible. KV pairs or something you could parse with regexp possible but there's no named standard you could trust, so it needs a separate function.

I'd be in favor of dropping the format argument and renaming this to ParseJSONObject.

@evan-bradley
Copy link
Contributor

Based on the discussion so far, I would agree that we should have a separate factory function for each format.

@TylerHelmuth
Copy link
Member Author

Ok based on this discussion and the precedence set by Stanza I'll update this function to be JSON specific

@TylerHelmuth TylerHelmuth changed the title [pkg/ottl] Add ParseToMap factory function [pkg/ottl] Add ParseJSON factory function Nov 23, 2022
@TylerHelmuth
Copy link
Member Author

@djaglowski @evan-bradley @bogdandrutu @kentquirk ready for re-review.

.chloggen/ottl-parse-to-map.yaml Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/README.md Outdated Show resolved Hide resolved
@github-actions github-actions bot requested a review from bogdandrutu November 28, 2022 23:38
pkg/ottl/ottlfuncs/func_parse_json.go Outdated Show resolved Hide resolved
@github-actions github-actions bot requested a review from bogdandrutu November 29, 2022 00:06
pkg/ottl/ottlfuncs/func_parse_json.go Outdated Show resolved Hide resolved
@github-actions github-actions bot requested a review from bogdandrutu November 29, 2022 01:11
pkg/ottl/ottlfuncs/func_parse_json.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_parse_json.go Outdated Show resolved Hide resolved
@TylerHelmuth TylerHelmuth added the ready to merge Code review completed; ready to merge by maintainers label Nov 29, 2022
@bogdandrutu bogdandrutu merged commit a743302 into open-telemetry:main Nov 29, 2022
@TylerHelmuth TylerHelmuth deleted the ottl-parse-to-map branch November 29, 2022 22:32
shalper2 pushed a commit to shalper2/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2022
* Add ParseToMap factory function

* add changelog entry

* run go mod tidy

* Switch to json-specific parser

* Update .chloggen/ottl-parse-to-map.yaml

Co-authored-by: Evan Bradley <[email protected]>

* Update example

* Apply feedback

* Reduce further

* Update pkg/ottl/ottlfuncs/func_parse_json.go

Co-authored-by: Bogdan Drutu <[email protected]>

* Fix lint

* Return error

* Update pkg/ottl/ottlfuncs/func_parse_json.go

Co-authored-by: Bogdan Drutu <[email protected]>

* Fix

Co-authored-by: Evan Bradley <[email protected]>
Co-authored-by: Bogdan Drutu <[email protected]>
@plantfansam plantfansam mentioned this pull request Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg/ottl ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants