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/stanza] Clean up split package #26544

Merged
merged 3 commits into from
Sep 11, 2023

Conversation

djaglowski
Copy link
Member

Subset of #26241

Follows #26540

  • Rename MultilineConfig to split.Config
  • Remove Multiline, previously a struct representation that only wrapped a split func
  • Remove NewMultilineConfig, because split.Config is just two simple fields with "" defaults.
  • Condense references in tests
  • Substantially increate test coverage in split package.

@github-actions github-actions bot requested a review from atoulme September 8, 2023 18:19
@djaglowski djaglowski force-pushed the pkg-stanza-split-cleanup branch 2 times, most recently from 96dc63f to 6df4bb6 Compare September 8, 2023 18:54
@djaglowski djaglowski force-pushed the pkg-stanza-split-cleanup branch from 6df4bb6 to 81eaee7 Compare September 8, 2023 18:59
@djaglowski djaglowski marked this pull request as ready for review September 8, 2023 19:35
@djaglowski djaglowski requested a review from a team September 8, 2023 19:35
MaxConcurrentFiles int `mapstructure:"max_concurrent_files,omitempty"`
MaxBatches int `mapstructure:"max_batches,omitempty"`
DeleteAfterRead bool `mapstructure:"delete_after_read,omitempty"`
SplitConfig split.Config `mapstructure:"multiline,omitempty"`
Copy link
Member

@dmitryax dmitryax Sep 11, 2023

Choose a reason for hiding this comment

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

We are not going to change how it's exposed to end-user as multiline, right?

Is there any other use cases where this config is being used? I mean where it's actually used for splitting rather then multiline which is rather combining.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, this doesn't change the user facing config at all.

 Is there any other use cases where this config is being used? I mean where it's actually used for splitting rather then multiline which is rather combining?

I'm not aware of any other uses outside of pkg/stanza.

I think the word "multiline" is a bit of a misnomer. What is really meant here is "splitting based on something other than newlines". This can mean splitting out multiple logs from one line, or reading in a single log that spans multiple lines. Either way, it is concerned more with splitting than combining. (The recombine operator is more directly meant for "combining" logs.)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I'm wondering if we can pick a word that works for splitting and combining so it can be exposed to the user with the same word but nothing good comes to my mind

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, not sure I agree that we should indicate combining here either way. It's all about splitting, just a question of whether we split on newlines or something else.

In terms of the actual term exposed to users in the config, I think multiline is not ideal, but in my experience it's been much more common to find logs that span multiple lines vs having multiple logs in a single line, so multiline is at least descriptive of the more common case. And of course logs that are naturally separated by newlines don't require configuration of the multiline section at all.

@djaglowski djaglowski merged commit ca8c1ad into open-telemetry:main Sep 11, 2023
@djaglowski djaglowski deleted the pkg-stanza-split-cleanup branch September 11, 2023 21:51
@github-actions github-actions bot added this to the next release milestone Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants