Skip to content
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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 76 additions & 1 deletion exporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}

Expand Down Expand Up @@ -366,6 +398,35 @@ func (b *Exporter) handleEvent(event Event) {
conflictingEventStats.WithLabelValues("counter").Inc()
}

case *FlushEvent:
Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Author

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. :)

Copy link
Contributor

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.

log.Debugf("FlushEvent")
for k, v := range b.Sets.store {
setevent := b.Sets.events[k]

b.handleEvent(&GaugeEvent{
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 |s events as the same.

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 case *GaugeEvent does.

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]
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

if found {
_, have := store[event.Value()]
if !have {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's customary in Go to collapse these lines to

if _, ok := store[event.Value()]; !ok {
	store[event.Value()] = true
}

so that it can go out of scope when no longer needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also see the variable usually called ok – you're calling it both have and found, I'd rather be consistent with those where possible.

Copy link
Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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

if … {
  … do stuff …
  return
}
… do other stuff …

rather than using an else. In most cases this means there is only one "ok" to work with. I don't see how to do that neatly here though, so I'm okay with leaving the names as they are.

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,
Expand Down Expand Up @@ -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),
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,5 +210,6 @@ func main() {
go watchConfig(*mappingConfig, mapper)
}
exporter := NewExporter(mapper)
exporter.Ticker(events)
exporter.Listen(events)
}