fix: prometheus with multiple providers

[iwpnd]

we encountered an issue where tegola would panic if you'd try to register duplicate metrics collectors.
that would happen if you have more than one mvt provider in the same config file.

this fix would append the provider name to the metric name. to ensure backward compatibility the provider name would only be appended if there were more than one.

Would also resolve #886

[coveralls]

Pull Request Test Coverage Report for Build 525de0799-PR-903

• 0 of 7 (0.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.01%) to 45.311%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
atlas/atlas.go	0	3	0.0%
atlas/map.go	0	4	0.0%

Totals
Change from base Build b358d4df0:	-0.01%
Covered Lines:	5745
Relevant Lines:	12679

---

💛 - Coveralls

[gdey]

I'm still looking at this. This was my initial idea (as well), but I want to make sure it makes sense. Also, why do we need both the array of prefixes and the hash map?

var prefixWithProvider bool
{
   var (
       mvtProviderSet = make (map[string]struct{})
      value = struct{}{}
   )
   for _, aMap := range a.maps {
        mvtProviderSet[aMap.mvtProviderName] = value
   }
   prefixWithProvider = len(mvtProviderSet) > 1
}
var prefix = "tegola"
for _, aMap := range a.maps {
     if prefixWithProvider {
        prefix = fmt.Sprintf("tegola_%s",aMap.mvtProviderName())
    }      
    collectors, err := aMap.Collectors(prefix,o.CollectorConfig)
   if err != nil {
       log.Errorf("failed to register collector (%s) for map: %s ignoring", prefix, aMap.Name)
      continue
   }

   if err = o.Register(collectors...); err != nil {
      log.Error("failed to register collectors %+v",err)
     panic(err)
   }
}

Would it be okay to break compatibility a bit here? Breaking compatibility will be changing the name of the observer key from tegola, to tegola_mvt_provider_name; if I'm reading this correctly. If willing, that could be handled in the postgis collector method.

I'm still tracing the code to make sure I understand what exactly is going on. Wanted to get my thoughts out as I have already dropped the ball on this once.

[iwpnd]

Cool, was not aware that Len() works on a map like that, which is why I'm jumping hoops there.

You're not breaking compatibility with this change, as previously it was not possible to track more than one provider with prometheus.

You are correct however, that ideally we'd re-use the same metric but distinguish different providers with a tag, similar to how you distinguish maps right now.

[gdey]

Looking through it, and taking into account your comments; I think the fix can be simpler.

[gdey]

After, digging deeper. I see what you mean that it would be a large refactor to get the mvt_provider name in, and I'm not sure; if it would work with how we are using labels. (Labels coming from the URL).

I don't think the suffix should be added here; instead, it should go into the Collector for the Map. There is no need to check to see if we only have one map, and only then suffix the mvt provider name. We can prefix the mvt provider name if it exists.

https://github.com/go-spatial/tegola/blob/master/atlas/map.go#L88-L94

Could be:

	if m.mvtProviderName != "" {
		collect, ok := m.mvtProvider.(observability.Observer)
		if !ok {
			return nil, nil
		}
		return collect.Collectors(prefix+"_"+m.mvtProviderName, config)
	}

[iwpnd]

Happy to comply here.

I was wondering however. Prometheus should be fine with reusing a metric, rendering a prefix unnecessary.
Say you have latency in your application. Every provider, every route should send metrics to latency with additional meta information (provider, path, map, ..). Separating this into meaningful insights is Grafanas job in the end.

It is done for the tegola_api_duration_seconds_bucket already:

tegola_api_duration_seconds_bucket{handler="/capabilities",method="get",le="+Inf"} 2
tegola_api_duration_seconds_bucket{handler="/capabilities/geometries-v2.json",method="get",le="+Inf"} 1

so ideally it would be:

tegola_mvt_provider_sql_query_seconds_bucket{mvt_provider="first_provider",map_name="geometries",z="10",le="0.1"} 11
tegola_mvt_provider_sql_query_seconds_bucket{mvt_provider="second_provider",map_name="geometries",z="10",le="0.1"} 11

Because on second thought, using a prefix comes with its on set of annoying perks for the user and tegola.
Do you create lowercase provider name prefix or leave it?
Do you create mutate my-provider-name to my_provider_name?

update 1:

panic: "tegola_first-provider_postgres_max_connections" is not a valid metric name

update 2:
your suggestion, adding the prefix works, if we account for invalid metric names. As labels would not care about skewed provider names, there's another reason to investigate how we can make that work here.

Do you suggest we go ahead with the prefix and fix the underlying issue of registering duplicate names in a separate step? If you do not share those concerns, forget what I wrote. :)

[gdey]

Sorry, for going dark.
I think we should investigate how to make labels work. That would be the better way.
This really should be a label, the more I thought about it the more I'm convinced of that.
We should avoid doing stop-gap fixes; which I think altering the metric name would be.

[iwpnd]

I found a solution using labels instead. I will close this PR and open a new one with a cleaner approach. Thank you anyway for taking the time and going through this with me @gdey 🙏