silently ignore certain unsupported metric types

[demmer]

Instead of logging a warning for the stats types which are known
to be incompatible with prometheus (e.g. String, Rates, etc), just
silently skip them in the publish phase.

At the same time remove the throttled logger and replace with a
warning log in case new metric types are created which aren't
covered as part of the backend.

Signed-off-by: Michael Demmer mdemmer@slack-corp.com

[michael-berlin]

How about log.Fatalf here instead?

Some background:

We have similar internal code.

In the past, we did only log a warning similar to this.

However, I plan to change this and issue a log.Fatalf() here instead. That's because a crash in a place like this would have caught the missed type handling I accidentally introduced in our internal code (when I adjusted it to the types refactor).

Given that, you probably want to do a similar thing here as well?

A panic here bears the risk that some code doesn't register their variable during init() and instead dynamically at runtime. But as long as each type has at least one variable instance which publishes during/near init(), we will detect such crashes in tests and not in production. That should be good enough?

For our internal plugins, I'll also guard this behavior by a flag and turn the flag off in our production instance config only. You could also add a flag here, but I would rather not set such a flag in e.g. our Kubernetes config. It's just too confusing.

Given all that, I suggest to just change this to log.Fatalf.

[demmer]

@michael-berlin I'm on board with that change and agree the future-proofing benefit is worth the risk.

I opted to not add another flag here and as a result we need to be vigilant when other stats types are refactored.

Then again until we did this work, I don't think anyone touched stats in the past year at least, so I think this is pretty safe all things considered.

[michael-berlin]

nit:

personal pet peeve of mine: Make this a proper sentence and use proper capitalization ("Prometheus'"):

// Silently ignore these types since they don't make sense to
// export to Prometheus' data model.

[demmer]

Done

[michael-berlin]

LGTM after minor comment about the error text is responded too.

You have merge rights as well, just merge at your own convenience :)

[michael-berlin]

nit: I like error messages to be actionable e.g. change it as follows?

"prometheus: Metric type %T (seen for variable: %s) is not covered by type switch. Add it there and to all other plugins which register a NewVarHook.", st, name)

[zmagg]

Missing a few:

JSONFunc, Float, FloatFunc, StringMap

FWIW, I just looked up everything that implemented String() inside the stats package, which is what's required for the expvar interface.

[demmer]

Oh good catch! I only checked for types that were actually used by the vitess code today.

@michael-berlin Since these types aren't used anywhere my instinct is that we should remove them from stats rather than either add them to this list or run the risk that someone starts using them in the future and then it will panic for anyone using prometheus.

[michael-berlin]

Confirmation that the following types are not used internally:

• JSONFunc
• Float
• StringMap

Types which must stay:

• FloatFunc

[sougou]

Is this good to go?

[demmer]

Given what Michael said I think we should add Prometheus exporter support for Float and FloatFunc since they are trivial, and remove the other unused types.

[sougou]

Sounds good. I'll wait.

[sougou]

Any update on this?

[demmer]

Ooooh. Totally forgot about this.

I just did the thing we talked about above and rebased off latest master.