-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat/re-allow multiple workers #36134
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
# Use this changelog template to create an entry for release notes. | ||
|
||
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
change_type: enhancement | ||
|
||
# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) | ||
component: prometheusremotewriteexporter | ||
|
||
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
note: Re allows the configuration of multiple workers | ||
|
||
# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. | ||
issues: [36134] | ||
|
||
# (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: | ||
|
||
# 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: [user] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,9 @@ type Config struct { | |
// maximum size in bytes of time series batch sent to remote storage | ||
MaxBatchSizeBytes int `mapstructure:"max_batch_size_bytes"` | ||
|
||
// maximum amount of parallel requests to do when handling large batch request | ||
MaxBatchRequestParallelism *int `mapstructure:"max_batch_request_parallelism"` | ||
|
||
// ResourceToTelemetrySettings is the option for converting resource attributes to telemetry attributes. | ||
// "Enabled" - A boolean field to enable/disable this option. Default is `false`. | ||
// If enabled, all the resource attributes will be converted to metric labels by default. | ||
|
@@ -87,6 +90,13 @@ var _ component.Config = (*Config)(nil) | |
|
||
// Validate checks if the exporter configuration is valid | ||
func (cfg *Config) Validate() error { | ||
if cfg.MaxBatchRequestParallelism != nil && *cfg.MaxBatchRequestParallelism < 1 { | ||
return fmt.Errorf("max_batch_request_parallelism can't be set to below 1") | ||
} | ||
if enableMultipleWorkersFeatureGate.IsEnabled() && cfg.MaxBatchRequestParallelism == nil { | ||
return fmt.Errorf("enabling featuregate `+exporter.prometheusremotewritexporter.EnableMultipleWorkers` requires setting `max_batch_request_parallelism` in the configuration") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to require people to set this. |
||
} | ||
|
||
if cfg.RemoteWriteQueue.QueueSize < 0 { | ||
return fmt.Errorf("remote write queue size can't be negative") | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -120,13 +120,18 @@ func newPRWExporter(cfg *Config, set exporter.Settings) (*prwExporter, error) { | |||||||||||||||||||||||
|
||||||||||||||||||||||||
userAgentHeader := fmt.Sprintf("%s/%s", strings.ReplaceAll(strings.ToLower(set.BuildInfo.Description), " ", "-"), set.BuildInfo.Version) | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
concurrency := cfg.RemoteWriteQueue.NumConsumers | ||||||||||||||||||||||||
if enableMultipleWorkersFeatureGate.IsEnabled() || cfg.MaxBatchRequestParallelism != nil { | ||||||||||||||||||||||||
concurrency = *cfg.MaxBatchRequestParallelism | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
Comment on lines
+123
to
+126
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
We want:
This suggestion accomplishes that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This way it will be a "change in behavior". There may be some users that set the Hence why I checked for the featuregate above to make sure *cfg.MaxBatchRequestParallelism was explicitly set by the user, and if not, default to I may have misunderstood but I thought the plan was to keep full retro compability. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will only be a change in behavior once the feature gate is enabled. It shouldn't be a breaking change in this PR (unless i'm mistaken). The reason we have a feature gate at all is to make the migration from the breaking change easier. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i missed the gate difference. ill update this over the weekend |
||||||||||||||||||||||||
|
||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice to always always use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. please check if you agree |
||||||||||||||||||||||||
prwe := &prwExporter{ | ||||||||||||||||||||||||
endpointURL: endpointURL, | ||||||||||||||||||||||||
wg: new(sync.WaitGroup), | ||||||||||||||||||||||||
closeChan: make(chan struct{}), | ||||||||||||||||||||||||
userAgentHeader: userAgentHeader, | ||||||||||||||||||||||||
maxBatchSizeBytes: cfg.MaxBatchSizeBytes, | ||||||||||||||||||||||||
concurrency: cfg.RemoteWriteQueue.NumConsumers, | ||||||||||||||||||||||||
concurrency: concurrency, | ||||||||||||||||||||||||
clientSettings: &cfg.ClientConfig, | ||||||||||||||||||||||||
settings: set.TelemetrySettings, | ||||||||||||||||||||||||
retrySettings: cfg.BackOffConfig, | ||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this value needs to default to
1
if this feature gate is enabled. When the feature gate is ultimately removed I don't think it appropriate to have a default value that is known to not work with any number of potential receivers and would not work with the default configuration of those receivers that are known to be able to support it in some configurations.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I aggree that will help with transition, and that the exporter should have "sane" defaults at all times.
done. please check