feat(inputs.statsd): Improve performance - #17872
Conversation
|
Thanks so much for the pull request! |
|
!signed-cla |
| err := parser.Init() | ||
| require.NoError(b, err) |
There was a problem hiding this comment.
Same for all other occurrences
| err := parser.Init() | |
| require.NoError(b, err) | |
| require.NoError(b, parser.Init()) |
| p := &graphite.Parser{Separator: s.MetricSeparator, Templates: s.Templates} | ||
| err := p.Init() | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Maybe extract in a new func which returns *graphite.Parser, error?
There was a problem hiding this comment.
I created it and used it also in tests
There was a problem hiding this comment.
Perfect, that was the idea.
|
|
||
| // newGraphiteParser initializes and returns a new graphite.Parser. graphite.Parser returned is not safe to be used in | ||
| // multiple goroutines. | ||
| func newGraphiteParser(s *Statsd) (*graphite.Parser, error) { |
There was a problem hiding this comment.
What about this?
| func newGraphiteParser(s *Statsd) (*graphite.Parser, error) { | |
| func (s *Statsd) newGraphiteParser() (*graphite.Parser, error) { |
There was a problem hiding this comment.
@Hipska one of the pipelines failed but I think it's not related to the changes
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
There was a problem hiding this comment.
Thanks for your contribution @radykal-com!
I think your code is racy for the case where another message is coming in via s.in in the parser function while we are currently using the parser. More specifically when setting p.DefaultTags in parseName().
If a second call (for another s.in message) arrives at that line before (or even worse while) the first call is executing the templates you will get mangled tags in your template result. We had this issue before and therefore introduced the lock.
What you could do though is to use a sync.Pool or parsers to be able to handle multiple messages in parallel while reusing the parser!
That cannot happen because each I'm pretty sure its working because the code is running in our high traffic metrics aggregator for the past week and is working fine (no panic) and the tests just pass OK with the |
|
@Hipska any chance to rerun the pipeline to get the failing one pass? |
srebhan
left a comment
There was a problem hiding this comment.
Thanks for your explanation @radykal-com! I agree with your assessment.
skartikey
left a comment
There was a problem hiding this comment.
@radykal-com Nice work! Thanks for the contribution!
Summary
Avoid using the mutex in each metric name parse by passing each worker goroutine its own
graphite.Parser.Checklist
Related issues
resolves #12313
Benchmark
There is a big difference in the overall throughput when using multiple threads
Old code
New code