[exporter][batching] configuration and config validation for bytes based batching #12154

sfc-gh-sili · 2025-01-22T03:23:45Z

Description

This PR adds config API that will be used for serialized bytes based batching.

We will deprecate MinSizeConfig and MaxSizeConfig in favor of:

type SizeConfig struct {
	Sizer string `mapstructure:"sizer"`
	MinSize int `mapstructure:"mix_size"`
	MaxSize int `mapstructure:"max_size"`
}

Link to tracking issue

#3262
#12303

Testing

Documentation

codecov · 2025-01-22T03:31:44Z

Codecov Report

Attention: Patch coverage is 68.42105% with 12 lines in your changes missing coverage. Please review.

Project coverage is 91.35%. Comparing base (83d93cd) to head (81d8227).

Files with missing lines	Patch %	Lines
exporter/exporterbatcher/sizer_type.go	66.66%	7 Missing and 1 partial ⚠️
exporter/exporterbatcher/config.go	71.42%	3 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (68.42%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12154      +/-   ##
==========================================
- Coverage   91.39%   91.35%   -0.04%     
==========================================
  Files         468      469       +1     
  Lines       25598    25636      +38     
==========================================
+ Hits        23395    23421      +26     
- Misses       1787     1797      +10     
- Partials      416      418       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bogdandrutu · 2025-01-22T04:07:40Z

exporter/exporterbatcher/config.go

 	MinSizeItems int `mapstructure:"min_size_items"`
+	MinSizeBytes int `mapstructure:"min_size_bytes"`


What if we have a "Sizer"("SizerType") as enum with 3 values (request, items, bytes) and the "size" value instead?

Good idea. How about something like this:

type BatcherConfig struct { ... minSize SizeConfig `mapstructure:"min_size"` maxSize SizeConfig `mapstructure:"max_size"` } type SizeConfig struct { sizer string `mapstructure:"sizer"` size int `mapstructure:"size"` } func (c SizeConfig) validate() { ... }

Does the option request mean that the request will implement its own sizing method?

Not sure if we need "sizer" in both min/max, since I don't think we need to accept different sizer for min and max.

I don't think we need to accept different sizer for min and max.

Good point. How about

type BatcherConfig struct { ... sizer SizeType `mapstructure:",squash"` minSize int `mapstructure:"max_size"` maxSize int `mapstructure:"max_size"` } type SizerType struct { sizer string `mapstructure:"sizer"` } func (c SizerType) validate() { }

Why is "Sizer" a configuration string? What does it name, and how does the user configure it?

I thought of something that encapsulates better. How about

type SizeConfig struct { Sizer string `mapstructure:"sizer"` MinSize int `mapstructure:"mix_size"` MaxSize int `mapstructure:"max_size"` }

That way we can validate SizeConfig pass it around as a whole.

@jmacd we are considering the options for the user Sizer -> "requests"|"items"|"bytes".

@sfc-gh-sili still need an enum with possible values for Sizer because usages of the SizeConfig need to do different things based on different types of sizers.

PS: I like the SizeConfig, but would still keep the SizerType :)

@bogdandrutu Sounds good! I updated the config to have BatchConfig -> SizeConfig -> SizerType. Would you mind taking another look?

bogdandrutu · 2025-01-31T18:24:10Z

exporter/exporterbatcher/config.go

+	err := c.SizeConfig.Validate()
+	if err != nil {
+		return err
+	}


You don't need to call Validate, it is called automatically.

Makes sense. Thanks!

bogdandrutu · 2025-01-31T18:24:30Z

exporter/exporterbatcher/config_test.go

+			MinSize: 100,
+		},
+	}
+	require.EqualError(t, cfg.Validate(), "sizer should either be bytes or items")


To test that it is called automatically, use component.ValidateConfig()

exporter/exporterbatcher/config.go

bogdandrutu · 2025-01-31T23:02:53Z

exporter/exporterbatcher/config.go

+
+type SizerType struct {
+	// Sizer should either be bytes or items.
+	Sizer string `mapstructure:"sizer"`


make this private and create an UnmarshalText (implement this interface https://pkg.go.dev/encoding#TextUnmarshaler). See https://github.com/open-telemetry/opentelemetry-collector/blob/main/pipeline/internal/globalsignal/signal.go

That way, we can control to only allow specific values.

bogdandrutu · 2025-01-31T23:03:14Z

exporter/exporterbatcher/config.go

+const SizerTypeItems = "items"
+const SizerTypeBytes = "bytes"


they should be SizerType type.

Makes sense.

bogdandrutu · 2025-01-31T23:03:33Z

exporter/exporterbatcher/config.go

+	if c.Sizer != SizerTypeItems && c.Sizer != SizerTypeBytes {
+		return errors.New("sizer should either be bytes or items")
+	}


Don't need this if you do what I suggest.

Thanks! I replaced Validate() with TextUnmarshal and updated the tests accordingly. Let me know if there is further issues

sfc-gh-sili · 2025-02-05T21:40:42Z

I noticed an existing issue that discusses the config API for queue #9462 and I think we should align batch config API and queue config API as possible.

Opened an issue #12303 for further discussion.

sfc-gh-sili requested a review from a team as a code owner January 22, 2025 03:23

sfc-gh-sili requested a review from mx-psi January 22, 2025 03:23

bogdandrutu reviewed Jan 22, 2025

View reviewed changes

sfc-gh-sili requested review from bogdandrutu and jmacd January 31, 2025 07:28

bogdandrutu reviewed Jan 31, 2025

View reviewed changes

sfc-gh-sili requested a review from bogdandrutu January 31, 2025 21:28

bogdandrutu reviewed Jan 31, 2025

View reviewed changes

jmacd mentioned this pull request Feb 3, 2025

RFC: Error transmission through batching components #11947

Closed

sfc-gh-sili force-pushed the sili-config-for-serialized-bytes-based-batching branch from 6669be4 to a39ba31 Compare February 4, 2025 00:40

sfc-gh-sili requested a review from bogdandrutu February 4, 2025 00:42

sfc-gh-sili force-pushed the sili-config-for-serialized-bytes-based-batching branch from 5c51d8b to fd15914 Compare February 4, 2025 07:25

This was referenced Feb 4, 2025

[exporter][batching] Serialized bytes based batching for logs #12288

Closed

[chore][exporter][batching] Serialized bytes based batching for logs #12299

Open

[exporterhelper][batcher] User interface for controling batch measurement #12303

Open

sfc-gh-sili added 8 commits February 7, 2025 17:12

Configuration and validation for bytes based batching

9eb3555

Update

9fe536b

update

556ffbc

Added another test

5264944

Made strign into constants

beaaa2b

Implement sizer text marshaler

e1e6655

Update go mod tidy

6211c51

go mod tidy

1fe0fe6

sfc-gh-sili force-pushed the sili-config-for-serialized-bytes-based-batching branch from 357d13c to 1fe0fe6 Compare February 8, 2025 01:12

Rename sizer.go to sizer_type.go

81d8227

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[exporter][batching] configuration and config validation for bytes based batching #12154

[exporter][batching] configuration and config validation for bytes based batching #12154

sfc-gh-sili commented Jan 22, 2025 •

edited

Loading

codecov bot commented Jan 22, 2025 •

edited

Loading

bogdandrutu Jan 22, 2025

sfc-gh-sili Jan 22, 2025 •

edited

Loading

bogdandrutu Jan 22, 2025

sfc-gh-sili Jan 22, 2025 •

edited

Loading

jmacd Jan 23, 2025

sfc-gh-sili Jan 31, 2025 •

edited

Loading

bogdandrutu Jan 31, 2025 •

edited

Loading

bogdandrutu Jan 31, 2025 •

edited

Loading

sfc-gh-sili Jan 31, 2025

bogdandrutu Jan 31, 2025

sfc-gh-sili Jan 31, 2025

bogdandrutu Jan 31, 2025

sfc-gh-sili Jan 31, 2025

bogdandrutu Jan 31, 2025

bogdandrutu Jan 31, 2025

sfc-gh-sili Feb 4, 2025

bogdandrutu Jan 31, 2025

sfc-gh-sili Feb 4, 2025

sfc-gh-sili commented Feb 5, 2025

		MinSizeItems int `mapstructure:"min_size_items"`
		MinSizeBytes int `mapstructure:"min_size_bytes"`

		const SizerTypeItems = "items"
		const SizerTypeBytes = "bytes"

[exporter][batching] configuration and config validation for bytes based batching #12154

Are you sure you want to change the base?

[exporter][batching] configuration and config validation for bytes based batching #12154

Conversation

sfc-gh-sili commented Jan 22, 2025 • edited Loading

Description

Link to tracking issue

Testing

Documentation

codecov bot commented Jan 22, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

sfc-gh-sili Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-sili Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-sili Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

bogdandrutu Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

bogdandrutu Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-sili commented Feb 5, 2025

sfc-gh-sili commented Jan 22, 2025 •

edited

Loading

codecov bot commented Jan 22, 2025 •

edited

Loading

sfc-gh-sili Jan 22, 2025 •

edited

Loading

sfc-gh-sili Jan 22, 2025 •

edited

Loading

sfc-gh-sili Jan 31, 2025 •

edited

Loading

bogdandrutu Jan 31, 2025 •

edited

Loading

bogdandrutu Jan 31, 2025 •

edited

Loading