Prometheus exporter

[paivagustavo]

This adds prometheus metrics exporter.

This was started by @shreyassrivatsan and I've updated it with the last updates of the metrics sdk and support for measures (histogram and summary).

[jmacd]

This is a good start! Thanks @paivagustavo. FYI @shreyassrivatsan we've taken this over, thanks for the initial draft and we'll keep you in the loop.

[jmacd]

The way this is written, we need to use the defaultkeys Batcher, which ensures that the labels on each export Record are the same. I think this is another reason to integrate the exporter with the controller (and let it construct a Batcher).

The defaultkeys Batcher will let us simplify this code, since we can be fairly sure that the calls to With() will match the tags declared for the vector.

[paivagustavo]

This method is not needed anymore so I have just deleted.

Do you mean this because of the labels.Encoded() call? because I've just moved it to another place.

I guess I should make the Prometheus exporter implements the LabelEncoder with a defaultkeys batcher for the moment?

[jmacd]

My understanding is that no two -Vec types can exist with the same name and different key sets. The defaultkeys batcher will ensure that the tag sets are always equal--that's why it makes sense as a default. I still envision other batchers being created to support greater configuration, but for now yes, I would use defaultkeys.

Also, yes, it makes sense to use the LabelEncoder interface to compute anything needed to map the -Vec type into the handle-like interface. If the map[string]string input to prometheus can be changed to a slice of values (as you said it could), then maybe the Encode() function will compute a slice of values.

[jmacd]

Looking good!

[shreyassrivatsan]

I think for now this can be in the exporter config. The user decides between histogram and summary at construction time.

In the future it would be nice to give a way to configure this on a per metric basis, but I am not very certain how that would looke..

[jmacd]

Yes, I'm interested in more flexible and configurable exporters. I believe there's a question as to whether such a configuration can be in any way shared by more than one exporter.

[paivagustavo]

I've added an option to change how measures are exported, defaulting to histograms.

[shreyassrivatsan]

maybe a separate loc for the *Vec methods?

[paivagustavo]

Sorry, I actually didn't understand your suggestion here, could you discuss a little further?

[jmacd]

It's not clear to me that this matters. Any label encoder should uphold the contract, which is that encodings are unique. I'm not sure it needs to be part of the options either. If we're not using the encoded form directly for export (if it's only a map key), any encoder should do and we should just take the encoding we get.

[paivagustavo]

Yeah, I've mixed things up.

[paivagustavo]

Now that I'm thinking about it. We talked about parsing the encoded labels instead of calling Emit on the labelValues method, to make that we would need to use a specific encoding. So I guess we would need this and remove option for changing the encoding or dropping this and the TODO about parsing labels.

For reference:

func labelValues(labels export.Labels) []string {
	// TODO(paivagustavo): parse the labels.Encoded() instead of calling `Emit()` directly
	//  this would avoid unnecessary allocations.
	values := make([]string, 0, labels.Len())
	for _, label := range labels.Ordered() {
		values = append(values, label.Value.Emit())
	}
	return values
}

[shreyassrivatsan]

i like this refactor..

this should probably return a pointer, no? the methods on the object rightly have a pointer receiver, so might as well return a pointer and reduce a copy here..

same applies to histograms and gauges too..

[jmacd]

These are placed in structs, so the pointer methods work. I'm happy with fewer pointers, fwiw.

[paivagustavo]

We could reduce a copy using pointer or save an alloc not using a pointer. In the end both should have little to no difference since exporters are created once.

Are you okay leaving this that way or do you have a strong objections against this?

[jmacd]

Let's leave this. In the Go base libraries, this sort of class is often accompanied by a "Must not be copied after first use" sort of comment--would that help?

[paivagustavo]

I actually don't know if that solves it. Copies could be done, it would just be a shallow copy, It is not like Mutex that a copy could have inconsistent and incorrect state.

[jmacd]

I don't see a problem, anyway.

[paivagustavo]

Thanks @shreyassrivatsan and @jmacd, for helping and reviewing this during the development. I've responded to all the open discussions.

I think we can now open this for others to review.

[jmacd]

I'd like to create a simple wrapper for this block of code, as mentioned in #345. It's OK if you think we should wait for a future PR.

[paivagustavo]

I think we should take that on a next PR, I would happily take it when this gets merged.

[jmacd]

Let's leave this. In the Go base libraries, this sort of class is often accompanied by a "Must not be copied after first use" sort of comment--would that help?

[ferhatelmas]

isn't a guard required if forEachError is nil or not ?

[paivagustavo]

We've being following the thought that if a single record fails to be exported that should not prevent the rest of the records to be processed.

There is still some questions we need to figure out about how to handle errors better on this ForEach method. One of the suggestions is to change its argument from func(export.Record) to func (export.Record) error to have a more uniform and simpler error handling on all exporters. But that is an open question that I think should be taken on a separate PR since it would touch not only this exporter but others too.

[rghetia]

@paivagustavo, I merged it to master but it failed due to #356 . Can you please fix the build? Thx.

[paivagustavo]

@rghetia just fixed it :)