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

Support for set type #184

wants to merge 1 commit into from

Conversation

bryanlarsen
Copy link

fixes #183

still todo:

  • flush interval option
  • support strings, not just floats
  • check for leaking goroutines during configuration reload

still todo:

- flush interval option
- support strings, not just floats
- check for leaking goroutines during configuration reload

Signed-off-by: Bryan Larsen <[email protected]>
Copy link
Contributor

@matthiasr matthiasr left a comment

Choose a reason for hiding this comment

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

This is a great start, thank you!

Please also remember to add tests for set events, including mappings with match_type: set.

}

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

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

store, found := b.Sets.store[metricName]
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.

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.

@bryanlarsen
Copy link
Author

Please also remember to add tests for set events, including mappings with match_type: set.

I did a quick look, but I didn't find a test to copy & modify to get me started. Can you point me at one?

@matthiasr
Copy link
Contributor

There is no test to copy&paste because we don't have this kind of time-delayed behavior yet. At the moment, we test two pieces separately: the mapping, and on a higher level the [event handling](https://github.com/prometheus/statsd_exporter/blob/master/bridge_test.

When you teach pkg/mapper about set events, add a test for the configuration around that. The existing event handling tests will cover the parsing of the set events. You will have to add a new test that sends several set events, then triggers a flush, verifies that the gauges are correct, sends more set events, flushes again, and verifies the gauge values again. I know this is a bit tedious to write, but if we don't have this test, the functionality will rot away very quickly (or be incorrect to begin with, how would we know?)

@matthiasr
Copy link
Contributor

@bryanlarsen are you still interested in working on this?

@bryanlarsen
Copy link
Author

We're not using this capability at the moment, so I haven't been working on it. It'd be nice to use it, but at this point you certainly can't count on me to finish this PR.

@matthiasr
Copy link
Contributor

Thanks, in that case I'll close it for now – if you or anyone else wants to pick it up, I'm happy to reopen it.

@matthiasr matthiasr closed this May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support "set" metric type
2 participants