performancecopilot / speed

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

PMID hash collisions #52

Closed lzap closed 6 years ago

lzap commented 6 years ago

Hey,

I am hitting a hard wall of 2^10 maximum metrics and I am getting collisions which are causing pmval: pmGetInDom(70.1560651): Unknown or illegal instance domain identifier when trying to read the values via CLI tool. I see it with just few dozens of metrics:

3 fm_rails_http_request_db_duration.hosts_controller.index
16 fm_rails_activerecord_instances.Location
16 fm_rails_ruby_gc_allocated_objects.environments_controller.index
23 fm_rails_http_request_view_duration.discovered_hosts_controller.index
23 fm_rails_ruby_gc_minor_count.subnets_controller.index
111 fm_rails_ruby_gc_major_count.environments_controller.index
156 fm_rails_activerecord_instances.Host__Managed
156 fm_rails_ruby_gc_count.domains_controller.index
171 fm_rails_http_request_total_duration.hosts_controller.get_power_state
171 fm_rails_ruby_gc_count.hosts_controller.show
340 fm_rails_http_requests.domains_controller.index
340 fm_rails_ruby_gc_freed_objects.compute_resources_controller.index
380 fm_rails_http_request_view_duration.api_v2_bookmarks_controller.index
380 fm_rails_ruby_gc_major_count.compute_resources_controller.index
999 fm_rails_http_requests.notification_recipients_controller.index
999 fm_rails_http_request_total_duration.hosts_controller.runtime

Possible solutions include explicit metric ID assignment instead of hash, that would perhaps require storing the ID in some "cache" file. Alternatively, there is plenty of bits in PMID in "cluster" but I am unsure what this is supposed to be for. In Speed, cluster seems to be bound to the client.

I need to implement support for instances to bring the number of metrics down to about a dozen and hope for no collisions. But I assume many users can be unlucky and symptoms are hard to track.

Edit: For the record here is the utility I generated PMIDs with (pipe through sort -n for best results):

 package main

    import (
      "fmt"
      "hash/fnv"
      "bufio"
      "os"
    )

    func hash(s string, b uint32) uint32 {
      h := fnv.New32a()

      _, err := h.Write([]byte(s))
      if err != nil {
        panic(err)
      }

      val := h.Sum32()
      if b == 0 {
        return val
      }

      return val & ((1 << b) - 1)
    } 

    func main() {
      scanner := bufio.NewScanner(os.Stdin)
      for scanner.Scan() {
          text := scanner.Text()
          fmt.Printf("%d %s\n", hash(text, 10), text)
      }
    }
lzap commented 6 years ago

After some doc reading, I realized that cluster cannot be used - it is to further distinguish different applications. So all MMV apps can only created maximum of 1024 metrics. Some detection might be useful perhaps.

fche commented 6 years ago

Can speed / libpcp_mmv open multiple mmv contexts, so as to use multiple cluster numbers, and thereby >> 1000 metrics total? Could you expose different statsd top-level metric namespaces into different mmv names/clusters?

natoscott commented 6 years ago

@lzap looking at your metrics, I recommend you use instances for your controllers rather than encoding them in the metric names. This is more efficient from all PCP client tools perspective, has a more efficient on-disk encoding when recorded, and opens you up the 32-bit instance identifier space (saving the relatively small PMID space for metrics that are common).

IOW, model your metrics more like this ...

fm_rails.http_request.db_duration.index[hosts_controller] fm_rails.http_request.total_duration.get_power_state[hosts_controller] fm_rails.http_request.total_duration.runtime[hosts_controller] fm_rails.ruby_gc_count.show[hosts_controller] fm_rails.ruby_gc_count.index[domains_controller] fm_rails.http_requests.index[domains_controller]

(this is the kind of naming I used in my previous role, previous employer, with Parfait instrumenting Java apps - so the approach is known to work well in practice)

I'm not 100% sure what .index and .show in your examples refer to though - it might instead make sense for you to use instance names like "index/hosts_controller" and "index/domain_controller".

I'd also recommend using the naming hierarchy more in your metric names, as shown above (make fm_rails the common sub-root, and trying to keep names consistent helps in the long run - e.g. not clear why you have 'http_request' vs 'http_requests' in some places).

suyash commented 6 years ago

Can speed / libpcp_mmv open multiple mmv contexts

A single PCPClient is a mapping to a single MMV Context. This was precisely done so applications can create multiple clients, in order to obtain multiple contexts.

The Cluster ID is calculated as a hash function of the name (https://sourcegraph.com/github.com/performancecopilot/speed/-/blob/client.go#L136), and has a max length of 12 bits. Since the entropy is so low, there are chances of collision.

But multiple MMV contexts are supported, by creating multiple clients using NewPCPClient.

lzap commented 6 years ago

Just to let you know, the "weird" underscore syntax from PCP standpoint is because I deliberately designed our telemetry API so it's easily compatible with both Prometheus and PCP. So for metric name I am using prometheus (underscore) syntax, then there's dot and the rest is prometheus label. Mapping example:

some_fixed_prefix + metric_name + "." + label_one + "." + label_two

You are right @natoscott that not using instances was a bad idea - I kinda assumed that MMV API allows me to ignore this in the first cut and I can revisit this later. Had no idea about 10bits for metric id. This looks like a non-trivial change. Damn.

lzap commented 6 years ago

Hmmm I struggle to figure out, how to add new instance into an existing instance metric. Everything seems to be designed as immutable. Any ideas? Let me explain. Whole statsd protocol is all dynamic, I get m1.i1 and after some time (hours) I get m1.i2 thus I want to grab metric named m1 and add new instance called i2.

suyash commented 6 years ago

Had no idea about 10bits for metric id

A metric is registered to a single MMV context (PCPClient), can always create multiple contexts to get more.

how to add new instance into an existing instance metric

The InstanceMetric constructor requires an InstanceDomain. Doing dynamic addition and removal of instances on an InstanceDomain object has been on my mind since GSoC ended ~1.5 years ago. Maybe this year we can pick this up.

For now, I guess you could create instance metrics with zero values for the instances, and update the values when they arrive.

lzap commented 6 years ago

Hm, it looks like creating multiple contexts is the only reasonable way. I can either create new contexts on collisions or take the remaining bits of the 32 bit hash up to possible 20 bits and use that to detect client. But this can eventually lead to as many as 4 millions of MMV contexts.

natoscott commented 6 years ago

| Hm, it looks like creating multiple contexts is the only reasonable way.

@lzap Use instance domains - they are the right solution here from PCP POV. If the Speed library doesn't support your needs here, extend it. In the long-term you will certainly regret trying to shoe-horn this into different clusters, its very limiting.

@suyash there are lots of options, and no need to wait for a student to hack on this. In order to reshape an instance domain on the fly, the MMV header timestamps need to be updated - the PMDA will re-evaluate the MMV mapping when they are both in sync once more.

lzap commented 6 years ago

Yeah, I agree. But I have no ambitions in digging into Speed library, so far everything was very new for me. I hacked together a small Python PMDA which grabs data from statsite, will likely continue that route: https://gist.github.com/lzap/3c610ac07f5d9c2eda1798658f7b4eec

lzap commented 6 years ago

I gave it a try and it was pretty easy to implement a client registry, it looks like 6 bits for cluster (64 maximum clients) is enough for me to avoid collisions. I can always implement proper mapping per-instance later:

https://github.com/lzap/pcp-mmvstatsd/commit/dad7a33d985301e3a2732d8cd8cd311b97dda3bc

Let me try this under some higher load. Thanks for help.

suyash commented 6 years ago

@lzap the hashing mechanism was used to be able to create multiple mmv contexts, while having the minimum possibility of collisions, one better alternative might be to allow users to set a global function that returns a cluster id, given a cluster name, which can default to the hashing mechanism. I'll try to take that up.

suyash commented 6 years ago

I hacked together a small Python PMDA which grabs data from statsite, will likely continue that route: https://gist.github.com/lzap/3c610ac07f5d9c2eda1798658f7b4eec

From an initial outlook, it looks like you're using a raw instance metric, which is available in speed through PCPInstanceMetric. The main advantage with PCPHistogram is that it provides an hdrhistogram as a backend, which provides some nice properties. I'll also try to replicate the implementation in speed

lzap commented 6 years ago

The Python PMDA relies on https://github.com/statsite/statsite which does provide histogram capability and more. The PMDA is just a dumb data loader, no magic there at all. On the other hand, it requires an extra process stuffing the FIFO from the other side, which is little bit more complex setup.