-
Notifications
You must be signed in to change notification settings - Fork 234
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
Support for set type #184
Support for set type #184
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -261,6 +261,25 @@ func (t *TimerEvent) Value() float64 { return t.value } | |
func (c *TimerEvent) Labels() map[string]string { return c.labels } | ||
func (c *TimerEvent) MetricType() mapper.MetricType { return mapper.MetricTypeTimer } | ||
|
||
type SetEvent struct { | ||
metricName string | ||
value float64 | ||
labels map[string]string | ||
} | ||
|
||
func (t *SetEvent) MetricName() string { return t.metricName } | ||
func (t *SetEvent) Value() float64 { return t.value } | ||
func (c *SetEvent) Labels() map[string]string { return c.labels } | ||
func (c *SetEvent) MetricType() mapper.MetricType { return mapper.MetricTypeGauge } | ||
|
||
type FlushEvent struct { | ||
} | ||
|
||
func (t *FlushEvent) MetricName() string { return "flush" } | ||
func (t *FlushEvent) Value() float64 { return 0.0 } | ||
func (c *FlushEvent) Labels() map[string]string { return map[string]string{} } | ||
func (c *FlushEvent) MetricType() mapper.MetricType { return mapper.MetricTypeGauge } | ||
|
||
type Events []Event | ||
|
||
type LabelValues struct { | ||
|
@@ -269,12 +288,25 @@ type LabelValues struct { | |
ttl time.Duration | ||
} | ||
|
||
type SetStore struct { | ||
store map[string]map[float64]bool | ||
events map[string]*SetEvent | ||
} | ||
|
||
func NewSetStore() *SetStore { | ||
return &SetStore{ | ||
store: make(map[string]map[float64]bool), | ||
events: make(map[string]*SetEvent), | ||
} | ||
} | ||
|
||
type Exporter struct { | ||
Counters *CounterContainer | ||
Gauges *GaugeContainer | ||
Summaries *SummaryContainer | ||
Histograms *HistogramContainer | ||
mapper *mapper.MetricMapper | ||
Sets *SetStore | ||
labelValues map[string]map[uint64]*LabelValues | ||
} | ||
|
||
|
@@ -366,6 +398,35 @@ func (b *Exporter) handleEvent(event Event) { | |
conflictingEventStats.WithLabelValues("counter").Inc() | ||
} | ||
|
||
case *FlushEvent: | ||
log.Debugf("FlushEvent") | ||
for k, v := range b.Sets.store { | ||
setevent := b.Sets.events[k] | ||
|
||
b.handleEvent(&GaugeEvent{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. won't this cause strange side effects? we allow matching on the metric type, so if I have a mapping that explicitly only applies to sets, it won't affect this event, but others will. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One thing you could to, instead of sending a GaugeEvent through the event handling again, is to maintain and set prometheus Gauge metrics directly, based on the mapping of the set event. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found myself duplicating much of the handleEvent function (to get mapping, help, labels), code which I don't completely understand yet. Doing it this way let me avoid the duplication. As far as I'm concerned, a set is a type of gauge; anything you'd want to do to a gauge (mapping, histograms as discussed, et cetera) probably would be useful on a set. So I also rationalize my laziness that way. It'd have to be called out in the documentation if so. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but these have all been already determined when mapping the set event. the mappings act on statsd types, not Prometheus types, so even if the output is a gauge we cannot treat I think the correct way is to preserve whatever is needed from the first event that is handled, and then get the Prometheus metric objects the same way |
||
relative: false, | ||
metricName: setevent.MetricName(), | ||
value: float64(len(v)), | ||
labels: setevent.labels, | ||
}) | ||
|
||
b.Sets.store[k] = map[float64]bool{} | ||
} | ||
|
||
case *SetEvent: | ||
store, found := b.Sets.store[metricName] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what happens if the event has dogstatsd tags attached? I think we need to keep separate sets for each of them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not familiar with dogstatsd, but that makes sense. Are those stored in Event.Labels()? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes |
||
if found { | ||
_, have := store[event.Value()] | ||
if !have { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's customary in Go to collapse these lines to
so that it can go out of scope when no longer needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also see the variable usually called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did it that way to avoid aliasing. Is aliasing generally not frowned on in Go code? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's probably better not to – although my instinct is that if you run into aliasing, you're nesting the code too deeply. One thing that took me a while to get used to in Go style is that we prefer
rather than using an |
||
store[event.Value()] = true | ||
} | ||
} else { | ||
b.Sets.store[metricName] = map[float64]bool{ | ||
event.Value(): true, | ||
} | ||
b.Sets.events[metricName] = ev | ||
} | ||
|
||
case *GaugeEvent: | ||
gauge, err := b.Gauges.Get( | ||
metricName, | ||
|
@@ -486,6 +547,7 @@ func NewExporter(mapper *mapper.MetricMapper) *Exporter { | |
Gauges: NewGaugeContainer(), | ||
Summaries: NewSummaryContainer(mapper), | ||
Histograms: NewHistogramContainer(mapper), | ||
Sets: NewSetStore(), | ||
mapper: mapper, | ||
labelValues: make(map[string]map[uint64]*LabelValues), | ||
} | ||
|
@@ -513,7 +575,11 @@ func buildEvent(statType, metric string, value float64, relative bool, labels ma | |
labels: labels, | ||
}, nil | ||
case "s": | ||
return nil, fmt.Errorf("no support for StatsD sets") | ||
return &SetEvent{ | ||
metricName: metric, | ||
value: float64(value), | ||
labels: labels, | ||
}, nil | ||
default: | ||
return nil, fmt.Errorf("bad stat type %s", statType) | ||
} | ||
|
@@ -642,6 +708,15 @@ type StatsDUDPListener struct { | |
conn *net.UDPConn | ||
} | ||
|
||
func (b *Exporter) Ticker(e chan<- Events) { | ||
ticker := time.NewTicker(10000 * time.Millisecond) | ||
go func() { | ||
for range ticker.C { | ||
e <- []Event{&FlushEvent{}} | ||
} | ||
}() | ||
} | ||
|
||
func (l *StatsDUDPListener) Listen(e chan<- Events) { | ||
buf := make([]byte, 65535) | ||
for { | ||
|
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'm not sure about handling the flush in-line with incoming statsd events. It's fundamentally different (internal, not user-generated. Would it make sense to break it out into its own loop, or does that get us into locking/mutex hell?
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.
if we keep it inline, please add a comment explaining why it's here
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'm not sure how much locking/mutex work there would be, I'm not super familiar with your code. This mechanism let me completely avoid the question. :)
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.
you would need to mutex-protect all the map accesses. it's fine this way, but I think we can't safely circle back into
handleEvent
because that would cause double-mapping.