-
Notifications
You must be signed in to change notification settings - Fork 233
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
Refactor Parser
into specialized parsers for each StatsD syntax
#576
Comments
Thank you Pedro! For context, this is the outcome of a discussion we had at
PromCon.
I consider 3 (not breaking internal APIs) a nice to have, as long as there
is an easy way to adapt for library users. The MultiParser would provide
that.
…On Thu, 12 Sept 2024, 22:08 Pedro Tanaka, ***@***.***> wrote:
Description
The current Parser implementation in pkg/line/line.go handles multiple
types of syntax (DogStatsD, InfluxDB, Librato, SignalFX) within a single
parser. This makes the code complex and harder to maintain. We propose
refactoring the Parser to separate parsers for each syntax type.
Goals
1. *Improve Code Maintainability*: By separating the parsing logic for
different syntax types, the code will be easier to understand and maintain.
2. *Allow for improvements for specific cases*: The current parser is
quite slow and heavy on allocations for parsing tags for Dogstatsd, for
example. We could after refactoring ensure that we optimize that specific
parser.
3. *Avoid Breaking Changes*: Ensure that the refactoring does not
break existing projects that use this repository as a library.
Proposed Solution
1. *Create Separate Parsers*: Implement individual parsers for
DogStatsD, InfluxDB, Librato, and SignalFX.
2. *Parser Interface*: Define a Parser interface that each specific
parser will implement.
3. *Stop-gap MultiParser*: Define a MultiParser that will mimic the
current behavior of the parser, and will check if the incoming line uses
which type of tagging. This is necessary, if your instance needs to
understand more than one type of syntax.
4. *Factory Method*: Implement a factory method to create the
appropriate parser based on configuration, here we could make sure to use
only the specific parser, if only one syntax is enabled.
—
Reply to this email directly, view it on GitHub
<#576>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABAEBTYZFWIJ5KYREGGOHLZWHYELAVCNFSM6AAAAABOD7Z2AOVHI2DSMVQWIX3LMV43ASLTON2WKOZSGUZDGMRWGA4TCMA>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Oh joy, I misremembered how we are currently treating tags when a specific flavor is disabled. If you send a line that uses a disabled tagging format, e.g. DogStatsD tags when DogStatsD parsing is disabled, we accept the line and ignore the tags. To get this behavior, we have to still parse for all formats. I think this is strange, and we should break it. It makes the parsing very inflexible and inefficient, for a use case that nobody ought to have. |
Description
The current
Parser
implementation inpkg/line/line.go
handles multiple types of syntax (DogStatsD, InfluxDB, Librato, SignalFX) within a single parser. This makes the code complex and harder to maintain. We propose refactoring theParser
to separate parsers for each syntax type.Goals
Proposed Solution
Parser
interface that each specific parser will implement.The text was updated successfully, but these errors were encountered: