[36] initial support for tagged graphite metrics #128
Conversation
Signed-off-by: glightfoot <[email protected]>
There was a problem hiding this comment.
this is a great start!
Strategically, I would like to make this exporter converge with the statsd exporter. Could you move the line/label parsing into a package similar to the line package there?
The label parsing should happen before invoking the mapper, so that the mappings match only the metric name.
What is the performance impact of the mutex around the cache? Are there parts of the statsd exporter we can re-use here to solve the inconsistent-label issue? I want the two to behave as similarly as possible. Semantically Graphite metrics with tags are the same as statsd gauges with dogstatsd/signalfx/librato tags, and I would prefer if the exporters don't add unnecessary differences on top.
|
Ah yeah I was afraid you'd say that, lol. I can work on breaking it up similar to the statsd exporter some next week, might take a while since I don't have too much capacity. For now, the mtx should have very little impact on performance since the exporter doesn't appear to be processing events concurrently (I haven't dug too deep into this code though, so I might be mistaken) and golang mutexes use atomic operations to update the lock. If this does end up becoming similar to the statsd exporter, the mutex will be necessary, so I thought it'd be safer to add it in now. I'm not sure how the statsd exporter handles inconsistent labels, so I'll have to look into that and see if it's even worth keeping track of. The bigger performance impact comes from getting the label keys, allocating a slice, sorting the slice, and then building a string from it to keep in the cache for comparisons. This was just to keep the client library happy with the gotcha in the issue, but I haven't tested any behavior with inconsistent labels |
Signed-off-by: glightfoot <[email protected]>
…ride parsed labels Signed-off-by: glightfoot <[email protected]>
|
I had some free time, so I took a stab at breaking this out into packages and moved the parsing to before the mapping. Not sure about the naming on the packages. Still has some issues to figure out, but at least do the packages make sense? |
glightfoot
force-pushed
the
tags
branch
3 times, most recently
from
d0af7bd
to
c3e6494
Compare
|
This got sloppier than I wanted trying to break things up into packages at the same time as the tagged metrics support. Let me know what you think about the general breakup. I still need to create some benchmarks to ensure that the breakup doesn't negatively affect performance and fix some tests |
…ckage Signed-off-by: glightfoot <[email protected]>
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package collector |
There was a problem hiding this comment.
I'm not entirely sure it's worth breaking this out into its own package... Can revert if necessary
|
Have you found a way to support underscores in metric names or label names. Have you tested this case? See #80 . |
|
It doesn't look too hard to support that, but this draft is already too big and will likely be broken up - I don't want to add any more functionality in here. Once some of the other PR's (e2e tests and benchmark) are merged, I'll be happy to take a look at it |
|
@matthiasr - This feels like too much change in one PR. After looking at the code some more, I think I could adapt this to use the same general structure as statsd_exporter (including processing samples in parallel), but that's a bigger change. How would you feel if I closed this draft and opened up a new one to just add tag support (since we have benchmarks now), then I can open a new issue to restructure the exporter and break it out into packages? |
|
That sounds awesome! Making the structures match, and re-using as much as possible from the statsd exporter, was my original vision when initiating the break-up there. Ideally, we can gradually reduce the graphite exporter to a main + line parser and otherwise use the same infrastructure. I am willing to break compatibility with existing graphite exporter behaviors if that is necessary to bring it in line with the statsd exporter. And if the packages need tweaking to do this we can. |
Initial hacking for supporting graphite tags.
This draft is very rough - I'll spend some time looking it over and making improvement, but wanted to get some feedback before going any further. I'm not too familiar with this exporter, so some of my assumptions may be wrong
In order to keep metric labels consistent, I added a simple cache of metric names and label keys previously encountered. Metrics sent with inconsistent label keys will be dropped and a metric incremented.
The e2e tests were already broken, so I didn't touch them.
Fixes #36
@matthiasr