Instrument descriptor semantics are unclear when using views

[aabmass]

What happened?

It's not clear to me if the instrument descriptor should represent the view or the original instrument when views are configured. When I tested it out, it seems to be a hybrid of both which is difficult to use when implementing an exporter.

Steps to Reproduce

Create a view to change a Counter instrument's aggregation to Histogram and rename it:

import {
  Aggregation,
  ConsoleMetricExporter,
  MeterProvider,
  PeriodicExportingMetricReader,
  View,
} from '@opentelemetry/sdk-metrics';

async function main() {
  const meterProvider = new MeterProvider({
    views: [
      new View({
        instrumentName: 'mycounter',
        aggregation: Aggregation.Histogram(),
        name: 'myrenamedhistogram',
      }),
    ],
  });
  meterProvider.addMetricReader(
    new PeriodicExportingMetricReader({
      exportIntervalMillis: 100,
      exporter: new ConsoleMetricExporter(),
    })
  );

  meterProvider.getMeter('foo').createCounter('mycounter').add(1);
  await new Promise<void>(resolve => {
    setTimeout(resolve, 150);
  });
}

main().catch(err => console.error('Error: %s', err));

Expected Result

I would expect either:

• the instrument descriptor exactly describes the original instrument (for understanding the source of the metric)
• the instrument descriptor describes a "synthetic" instrument that could have generated the metric. In the above example, the type would be HISTOGRAM.

Actual Result

Somewhere in between; the descriptor's name is changed to match the view, but the type is still COUNTER:

{
  descriptor: {
    name: 'myrenamedhistogram',
    description: '',
    type: 'COUNTER',
    unit: '',
    valueType: 1
  },
  dataPointType: 0,
  dataPoints: [
    {
      attributes: {},
      startTime: [Array],
      endTime: [Array],
      value: [Object]
    }
  ]
}

Additional Details

OpenTelemetry Setup Code

No response

package.json

$ npm list | grep '@opentelemetry'
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]
├── @opentelemetry/[email protected]

Relevant log output

No response

[dyladan]

the instrument descriptor exactly describes the original instrument (for understanding the source of the metric)

I think this is the intended behavior but we should look into other SIGs

[aabmass]

I don't see any Instrument Descriptor exposed in Java's MetricData (exporters receive a Collection<MetricData>). Ik we don't have it in Python either. Under Go's ResourceMetrics I don't see a dedicated instrument descriptor. I see that Metrics.Name's comment says "Name is the name of the Instrument that created this data" but I imagine the comment is inaccurate.

Important to note that instrument descriptor is not a part of the OTel data model in general.

[aabmass]

Looking into this a bit more, the InstrumentDescriptor looks like the only place the metric name is available.. I'm not sure where we would indicate the instrument name vs the view name in that case.

[pichlermarc]

It looks that I made a mistake while I was working on #3158. 😞
I must have overlooked the descriptor InstrumentType. Unfortunately, the same problem stated here will also affect the instrument unit and description.

What descriptor in this case is supposed to represent is a "virtual" instrument created by a view, and it should not have an InstrumentType representing it, as it is to no concern of the exporter which type of instrument produced the data. Unfortunately, we cannot change this to map to a "virtual" instrument type, as it is not mappable 1:1. Histograms for instance are not allowed negative values, but an UpDownCounter or ObservableGauge with an ExplicitBucketHistogramAggregation is, so setting the type to Histogram in that case would not be accurate.

As we already released @opentelemetry/sdk as stable, I fear that there is not much we can do to change this anymore. My proposal is that we mark this as deprecated, we document it in a way that makes it clear that this represents the original instrument's type.

Sorry about this, I can see that this can be confusing for exporter authors. After reviewing the code in our repo here, I can even see an instance of this being improperly used internally in the prometheus exporter. 😕

[aabmass]

As we already released @opentelemetry/sdk as stable, I fear that there is not much we can do to change this anymore. My proposal is that we mark this as deprecated, we document it in a way that makes it clear that this represents the original instrument's type.

+1 to some kind of deprecation. Do you mean deprecating the BaseMetricData.descriptor property altogether and moving the needed fields into BaseMetricData, or just the InstrumentDescriptor.type field? If you meant the latter, I would also like to rename InstrumentDescriptor -> Descriptor to avoid the confusion with instruments if possible.

Sorry about this, I can see that this can be confusing for exporter authors. After reviewing the code in our repo here, I can even see an instance of this being improperly used internally in the prometheus exporter. 😕

No worries at all thanks for working on this 😄

[pichlermarc]

As we already released @opentelemetry/sdk as stable, I fear that there is not much we can do to change this anymore. My proposal is that we mark this as deprecated, we document it in a way that makes it clear that this represents the original instrument's type.

+1 to some kind of deprecation. Do you mean deprecating the BaseMetricData.descriptor property altogether and moving the needed fields into BaseMetricData, or just the InstrumentDescriptor.type field?

I meant the latter, yes. Just deprecating the InstrumentDescriptor.type.

If you meant the latter, I would also like to rename InstrumentDescriptor -> Descriptor to avoid the confusion with instruments if possible.

I agree having it renamed would be good. I'll see if I can come up with a solution and will link the PR here 🙂

[legendecas]

I agree we should deprecate the usage of MetricData.descriptor.type in preference of MetricData.dataPointType. Exporters should not depend on the instrument type as the aggregator can be mapped to arbitrary ones by views.

Renaming InstrumentDescriptor to Descriptor sounds good to me.

[pichlermarc]

We've made some progress on this, and have introduced MetricDescriptor (instead of the previously agreed upon Descriptor).

PR #5266 now drops the InstrumentDescriptor in favor of MetricDescriptor, and the next step of the process is moving the type from MetricDescriptor to the now-internal InstrumentDescriptor which would finally address this issue.

[pichlermarc]

I'm adding this to the 2.0 Milestone as removing deprecated properties is in scope.