Skip to content

[pkg/stanza] Clean up split package #26544

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

Merged
merged 3 commits into from
Sep 11, 2023
Merged

[pkg/stanza] Clean up split package #26544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c4de85e
[pkg/stanza] Rename tokenize package to split
djaglowski Sep 8, 2023
81eaee7
[pkg/stanza] Clean up split package
djaglowski Sep 8, 2023
6e5b916
Merge branch 'main' into pkg-stanza-split-cleanup
djaglowski Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions .chloggen/pkg-stanza-rm-tokenize.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: breaking

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: pkg/stanza

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Rename "tokenize" package to "split"

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [26540]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext: |
- Remove 'Multiline' struct
- Remove 'NewMultilineConfig' struct
- Rename 'MultilineConfig' to 'split.Config'

# If your change doesn't affect end users or the exported elements of any package,
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: [api]
37 changes: 18 additions & 19 deletions pkg/stanza/fileconsumer/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import (
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/fileconsumer/matcher"
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/operator"
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/operator/helper"
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/tokenize"
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/split"
"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/trim"
)

Expand Down Expand Up @@ -53,7 +53,6 @@ func NewConfig() *Config {
IncludeFileNameResolved: false,
IncludeFilePathResolved: false,
PollInterval: 200 * time.Millisecond,
Multiline: tokenize.NewMultilineConfig(),
Encoding: defaultEncoding,
StartAt: "end",
FingerprintSize: fingerprint.DefaultSize,
Expand All @@ -66,22 +65,22 @@ func NewConfig() *Config {
// Config is the configuration of a file input operator
type Config struct {
matcher.Criteria `mapstructure:",squash"`
IncludeFileName bool `mapstructure:"include_file_name,omitempty"`
IncludeFilePath bool `mapstructure:"include_file_path,omitempty"`
IncludeFileNameResolved bool `mapstructure:"include_file_name_resolved,omitempty"`
IncludeFilePathResolved bool `mapstructure:"include_file_path_resolved,omitempty"`
PollInterval time.Duration `mapstructure:"poll_interval,omitempty"`
StartAt string `mapstructure:"start_at,omitempty"`
FingerprintSize helper.ByteSize `mapstructure:"fingerprint_size,omitempty"`
MaxLogSize helper.ByteSize `mapstructure:"max_log_size,omitempty"`
MaxConcurrentFiles int `mapstructure:"max_concurrent_files,omitempty"`
MaxBatches int `mapstructure:"max_batches,omitempty"`
DeleteAfterRead bool `mapstructure:"delete_after_read,omitempty"`
Multiline tokenize.MultilineConfig `mapstructure:"multiline,omitempty"`
TrimConfig trim.Config `mapstructure:",squash,omitempty"`
Encoding string `mapstructure:"encoding,omitempty"`
FlushPeriod time.Duration `mapstructure:"force_flush_period,omitempty"`
Header *HeaderConfig `mapstructure:"header,omitempty"`
IncludeFileName bool `mapstructure:"include_file_name,omitempty"`
IncludeFilePath bool `mapstructure:"include_file_path,omitempty"`
IncludeFileNameResolved bool `mapstructure:"include_file_name_resolved,omitempty"`
IncludeFilePathResolved bool `mapstructure:"include_file_path_resolved,omitempty"`
PollInterval time.Duration `mapstructure:"poll_interval,omitempty"`
StartAt string `mapstructure:"start_at,omitempty"`
FingerprintSize helper.ByteSize `mapstructure:"fingerprint_size,omitempty"`
MaxLogSize helper.ByteSize `mapstructure:"max_log_size,omitempty"`
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 •
edited
Loading

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

@djaglowski djaglowski Sep 11, 2023

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

@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.

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

@djaglowski djaglowski Sep 11, 2023

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.

TrimConfig trim.Config `mapstructure:",squash,omitempty"`
Encoding string `mapstructure:"encoding,omitempty"`
FlushPeriod time.Duration `mapstructure:"force_flush_period,omitempty"`
Header *HeaderConfig `mapstructure:"header,omitempty"`
}

type HeaderConfig struct {
Expand All @@ -101,7 +100,7 @@ func (c Config) Build(logger *zap.SugaredLogger, emit emit.Callback) (*Manager,
}

// Ensure that splitter is buildable
factory := splitter.NewMultilineFactory(c.Multiline, enc, int(c.MaxLogSize), c.TrimConfig.Func(), c.FlushPeriod)
factory := splitter.NewMultilineFactory(c.SplitConfig, enc, int(c.MaxLogSize), c.TrimConfig.Func(), c.FlushPeriod)
if _, err := factory.Build(); err != nil {
return nil, err
}
Expand Down
Loading