Skip to content

Increasing the metric buffer#8145

Merged
ssoroka merged 4 commits intoinfluxdata:masterfrom
M0rdecay:execd_buffer
Sep 18, 2020
Merged

Increasing the metric buffer#8145
ssoroka merged 4 commits intoinfluxdata:masterfrom
M0rdecay:execd_buffer

Conversation

@M0rdecay
Copy link
Copy Markdown
Contributor

@M0rdecay M0rdecay commented Sep 17, 2020

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Closes #8142
Also related with #8121

ssoroka
ssoroka reviewed Sep 17, 2020
View reviewed changes
plugins/processors/execd/execd.go Outdated
Comment on lines +27 to +29
## Buffer size in bytes for storing metrics received from the process
## The minimum allowed value is 4096
buffer_size = 65536
Copy link
Copy Markdown
Contributor

@ssoroka ssoroka Sep 17, 2020

Choose a reason for hiding this comment

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

this is actually the buffer size used for storing a single metric, not multiple metrics.

ssoroka
ssoroka suggested changes Sep 17, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

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

I'm still not sure I like that it's configurable. we should just do the right thing. Too many knobs and dials to configure is just confusing.

This could literally be a two line change that works for 100% of people. No configuration options.

@M0rdecay
Copy link
Copy Markdown
Contributor Author

M0rdecay commented Sep 17, 2020 •
edited
Loading

I'm still not sure I like that it's configurable. we should just do the right thing. Too many knobs and dials to configure is just confusing.

This could literally be a two line change that works for 100% of people. No configuration options.

What about these values?
Hope this will be enough (unfortunately i don't really know how large metrics i have yet to see)...

	scanner := bufio.NewScanner(out)
	scanBuf := make([]byte, 4096) // default size
	scanner.Buffer(scanBuf, 262144) // max size

@M0rdecay M0rdecay changed the title Configurable execd processor buffer size Increasing the metric buffer Sep 18, 2020
@M0rdecay M0rdecay requested a review from ssoroka September 18, 2020 17:42
ssoroka
ssoroka approved these changes Sep 18, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

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

Looks good.

@ssoroka ssoroka merged commit 39f4c36 into influxdata:master Sep 18, 2020
@ssoroka
Copy link
Copy Markdown
Contributor

ssoroka commented Sep 18, 2020

You may run into size issues in other places, but at least this won't be one of them.

@M0rdecay
Copy link
Copy Markdown
Contributor Author

M0rdecay commented Sep 18, 2020

You may run into size issues in other places, but at least this won't be one of them.

Yes, I understand, but for now we prefer to use an external processor for working with XML - until a built-in parser appears in the telegraf.

I tested processing larger documents (10-12 thousand lines), everything is fine with the new buffer.
Also, there are no errors when processing such metrics when using an assembly with the parser inside the agent.

Many thanks for the help!

idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
@M0rdecay
Increasing the metric buffer (influxdata#8145)
c7f045d
meseeif
meseeif reviewed Nov 9, 2020
View reviewed changes
plugins/processors/execd/README.md Outdated

## Buffer size in bytes for storing metrics received from the process
## The minimum allowed value is 4096
# buffer_size = 65536
Copy link
Copy Markdown

@meseeif meseeif Nov 9, 2020

Choose a reason for hiding this comment

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

its the same when execd used as inputs plugin？

arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
@M0rdecay
Increasing the metric buffer (influxdata#8145)
4514383
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@meseeif meseeif meseeif left review comments

@ssoroka ssoroka ssoroka approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[processors.execd] Make the buffer size configurable

3 participants

@M0rdecay @ssoroka @meseeif