performancecopilot / speed

A Go implementation of the PCP instrumentation API
MIT License
37 stars 6 forks source link

Reuse instances for histograms #54

Closed lzap closed 6 years ago

lzap commented 6 years ago

Hey,

we use Speed to report about a thousand of histograms, this creates about a thousand of min,max,mean,std_dev instances. I am under impression that it should be possible to create those instances just once and then reuse their IDs.

If I am wrong just close the RFE ticket, sorry for the noise :-)

suyash commented 6 years ago

we use Speed to report about a thousand of histograms, this creates about a thousand of min,max,mean,std_dev instances. I am under impression that it should be possible to create those instances just once and then reuse their IDs.

To create a Histogram, you'll need to specify a name, and the name needs to be unique for each metric since the metric name generation hash isn't randomized and depends on the string, so same strings will hash to the same value. For each metric, the instance names are "min", "max" ... and the ID generation for them uses the same mechanism. (https://github.com/performancecopilot/speed/blob/master/instance.go#L29), so IDs would be shared (because all of them will have the same name).

The problem here is that the PCPHistogram constructor does not accept an InstanceDomain as a constructor. For each histogram, a new InstanceDomain is created, and hence the same instances are written twice, albeit for different instance domains, see the screenshot of an instance block with two histograms generated using mmvdump.

untitled

The ideal scenario to share instances would be to share instance domains, across different metrics, which the raw NewPCPInstanceMetric constructor allows to do, but custom metrics don't.

In my opinion, using a single global InstanceDomain object for all histograms would be an ideal fix, since for histograms we are not allowing customization anyways and the instances available are shipped with the library itself.

@natoscott thoughts?

natoscott commented 6 years ago

@suyash yes, definitely a good idea to enable this global sharing - looking with @lzap at some recorded data we had ~600+ duplicate histogram indoms on one day. :)

lzap commented 6 years ago

Thank you, since this can make Speed much more effective, I am going to build new release with rebased Speed library with the patch once merged.