Metric stdout NewExportPipeline should use ungrouped Batcher

[jmacd]

The defaultkeys batcher doesn't print complete information, which seems like what the user of a stdout exporter wants. The ungrouped batcher prints complete information.

[lizthegrey]

I suggested this to @rebeccajae as a good first issue...

[rebeccajae]

I switched the stdout exporter to use the ungrouped batcher, and it only outputs bound instruments. For example, using the basic example in example/basic the name changes from

ex.com.one{ex.com/foo=,ex.com/bar=,ex.com/lemons=10}

to

ex.com.one{A=1,B=2,C=3,ex.com/lemons=10}

This isn't including anything from the gauge options - that is, whatever is provided in WithKeys will not be in the name. So this may also be a bug in the batcher too.

[jmacd]

With the defaultkeys Batcher you get the three keys passed in the WithKeys() option, but two of them have empty/unspecified values.

With the ungrouped batcher, you get four keys that were actually passed in the commonLabels := meter.Labels(...) statement. I believe the ungrouped behavior is more useful in a stdout use-case where you are likely to read the labels, whereas the defaultkeys behavior is more appropriate when metrics are being aggregated and exported into a metrics system.

[lizthegrey]

But my reading is that the values aren't unspecified? Or maybe there's a bug in the example code or my understanding.

[rebeccajae]

I feel like I got caught up on "complete information" - the ungrouped batcher provides different information which may be more useful based on context. I was mentioning this because neither seemed to provide complete information, but a different subset of the overall available keys.

[jmacd]

I see. I think you're right.

One nice thing about ungrouped is that it's less expensive since it passes the label set through.

There is a spec-level discussion about this topic (open-telemetry/opentelemetry-specification#345), and it appears that we are collectively leaning toward letting the empty string and the unspecified value be treated the same.

It is in that sense when I say that ungrouped prints "complete" information, since any not-present key can be considered unspecified and we need not print them. I do believe the API specification should state that keys passed to the instrument WithKeys(...) are explicitly unspecified by implication in the event. In the SDK specification (WIP open-telemetry/opentelemetry-specification#347), I would state that the ungrouped Batcher (a.k.a. Integrator in newer terms) is not required to explicitly list the WithKeys unspecified values, simply because it comes at some expense.

It's possible to handle this on a case-by-case basis in an exporter. The exporter has access to the instrument descriptor, so we need not "materialize" unspecified keys before the exporter.

Let's add to this ticket that the stdout exporter should materialize these itself, by scanning through the WithKeys and printing any that are unspecified in some distinct way (as opposed to the key= empty string). Does that sound good?

[rebeccajae]

So the way I see it is dependent on who/what is intended to read the output of this exporter. Currently it conforms to the Prometheus spec of using an empty string to represent null/empty values. This, however has the effect of not being able to know if the value is null or the key is unspecified.

If this exporter is intended to be human-readable, then it may be worthwhile to come up with a distinct way of showing these. An idea that I had was to print them as part of the name, but without the = at all, so given the example where the last two keys are unspecified -

ex.com.one{A=1,B=2,C=3,ex.com/lemons=10,ex.com/foo,ex.com/bar}

Alternately, this could be rendered using a separate unspecified field that contains a list of unspecified keys in the output.

...
"name" : "ex.com.one{A=1,B=2,C=3,ex.com/lemons=10}",
"unspecified" : ["ex.com/foo", "ex.com/bar"],
...

[jmacd]

Both of these suggestions are good. I'd choose the first suggestion, using = to indicate presence. I'm interested in what others think. Thanks!

[paivagustavo]

I second that both options sounds good and also have a preference for the first one as well.

[lizthegrey]

Sorry this turned out more complicated than a trivial true->false or import change :)

as you can see, the spec and code indeed are evolving, so thank you @rebeccajae :)

[lizthegrey]

Fixed by #436, thank you for your first contribution @rebeccajae!