-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add Carbon Plaintext #10
base: master
Are you sure you want to change the base?
Conversation
carbon/datapoint.go
Outdated
// the value will be None (null). | ||
// Doc: https://graphite.readthedocs.io/en/latest/terminology.html | ||
type Datapoint struct { | ||
path string |
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 would suggest to use []bytes or bytes.Buffer here instead. Main reason is that currently WithTag
, WithPrefix
methods would cause a lot of unneeded allocations (strings in golang are immutable).
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.
Yeah, you're right! The optimization won't be premature this time
carbon/datapoint.go
Outdated
} | ||
|
||
// Plaintext returns Graphite Plaintext record form of datapoint | ||
func (d *Datapoint) Plaintext() string { |
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.
It would be better to call method String
as that's more go-way of doing things.
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.
Renamed it, but i need to think about this method, looks bit frustrating to me now
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.
Renamed path to buffer to make things more clear
carbon/datapoint_test.go
Outdated
import "testing" | ||
|
||
func Test_NewDatapoint(t *testing.T) { | ||
const ( |
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.
It would be cleaner to use table driven tests:
https://github.com/golang/go/wiki/TableDrivenTests
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.
Fixed
I'm not sure it would be useful to a lot of people, but I don't have anything against merging that, except for the comments I put in the PR. However I would maybe ask other people about their opinion on merging it (after the comments are fixed, plus maybe they will have more to say) |
…mples more carbonapi-sh, add benchmarks
Maybe there should be something similar to this one: |
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 have similar feedback as @Civil does. No concerns or objections from my side.
I guess maybe some users might like to use a library for generating graphite metrics.
Thanks for the contribution.
I'm so sorry, i've just realized there can be a better way to send the datapoints, so i'll push a lot of changes now |
Added metric builder, so now you can build metrics first, and then create datapoints when it's necessary |
Added simple Carbon Datapoints builder, so following workaround can be used:
generic service with pre-created metrics:
generic tcp datapoint writer:
generic http datapoint reader:
The use cases i thought of are: