Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Need clarity on scope of Instrument Name #1366

Closed
victlu opened this issue Jan 22, 2021 · 10 comments
Closed

Need clarity on scope of Instrument Name #1366

victlu opened this issue Jan 22, 2021 · 10 comments
Assignees
Labels
area:data-model For issues related to data model priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:metrics Related to the specification/metrics directory

Comments

@victlu
Copy link
Contributor

victlu commented Jan 22, 2021

Instruments with same namespace / names

Meter implementations MUST return an error when multiple instruments are registered by the same name.

Example

A service installs OTel SDK and uses a 3P library (instrumented with OTel). The service is required to initializing the 3P library for each new transaction. This causes the 3P library to register the instrument (with same namespace and instrument name). Thus, resulting in failure except for 1st instance.

How should this situation be resolved?

Expectation

When 3P library calls New-Instrument(Name) from multiple instances (in scope of one OTel SDK instance), I would expect the SAME instrument to be returned. Thus, all instances of 3P library will add/record measurements to the singleton "namespace/name" instrument.

Different instrument type with same name

Metric instrument names belong to a namespace, established by the the associated Meter instance.

Example

Create Counter and UpDownCounter and ValueRecorder with the same instrument name. Are they the same instrument or are they 3 different instruments (differentiated by kind/group)? Are Counter and UpDownCounter one or two distinct instruments (both are technically counters)?

Expectation

Need to incorporate "Kind" (or groupings) of instrument when establishing distinctness of instruments.

Implementation of "number-type" specific instruments

Example

C# SDK (other SDKs as well) incorporates number-types (i.e. "Int64", "Double") into the API when creating an instrument. (i.e. CreateInt64Counter(), CreateDoubleCounter()). If these are given the same instrument name, are they distinct and different instruments?

Expectation

Number-Type distinction should not be part of the API name. The APIs (i.e. Add/Record) should accept different number-types as parameters (for languages that are sensitive to different number types).

Also, conversions/promotions of number-types needs to be specified (i.e. int64 can be promoted to double, etc...) when summing or summarizing or aggregating results. This may be specific to SDK implementations.

@victlu victlu added the spec:metrics Related to the specification/metrics directory label Jan 22, 2021
@jmacd
Copy link
Contributor

jmacd commented Jan 26, 2021

These are all good questions. Given our recent discussions in the Metric working group(s), I'd like to try and re-cast these questions in terms of the protocol and not the API and SDK behavior. Suppose the collector is presented with more than one kind of OTLP data point for a metric with the same name. How should it treat that data? I believe the best approach is to treat these as distinct, i.e., Counter and UpDownCounter of the same name are different.

Likewise for Int vs Floating-point considerations. Some languages don't have much control over number type, and even when the programmer does have a choice, it is likely that a collector or OTLP service will see metrics with the same name and point kind but different number type. What should we do? I'd like to see services coerce their data to make this work.

@victlu
Copy link
Contributor Author

victlu commented Jan 27, 2021

Any recommendation on how to start this conversation in the community and get a resolution?

@andrewhsu andrewhsu added priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA area:api Cross language API specification issue labels Feb 5, 2021
@hdost
Copy link

hdost commented Feb 11, 2021

Meter implementations MUST return an error when multiple instruments are registered by the same name.

So if you're registering a Counter and an UpDownCounter then it should fail. That make sense as each Instrument named the same should be of the same type.

The assumption I have made is that 3P library would want to necessarily name their metrics the same and to that I could agree that if both a user of a library is looking to write certain metrics and the library uses the same metric name, or potentially two different libraries tried to use the same then that would be problematic.

However, that is something that should not be encouraged. We do need to keep in mind that not just because of OLTP, but also compatibility with OpenMetrics should likely be considered. These instruments which in the OpenMetrics and Prometheus formats result in a metric family is expected to be of the same type. If you're performing calculations on one vs the other the expectations are different. I would perform a rate calculation on a Counter but not an UpDownCounter.

On a similar point:

Are Counter and UpDownCounter one or two distinct instruments (both are technically counters)?

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#metric-instruments
The spec does actually provide information into the differences.

@aabmass
Copy link
Member

aabmass commented Feb 11, 2021

I think we should probably say there must be only one instrument with a given name registered to a given meter. I don't think we should allow/encourage an instrumentation to create a e.g. ValueRecorder and Counter with the same name on its meter (or is there some use case for this I'm missing?). Calling Meter.GetInstrument() with the same name but a different overall identity (e.g. float instead of int) would raise an error. Then the question is whether calling Meter.GetInstrument() with the same identity should return a reference to the existing instrument (I think that would be fine).

The assumption I have made is that 3P library would want to necessarily name their metrics the same and to that I could agree that if both a user of a library is looking to write certain metrics and the library uses the same metric name, or potentially two different libraries tried to use the same then that would be problematic.

@hdost It is actually encouraged for multiple libraries to create instruments with the same name (and overall identity) because of the semantic conventions. E.g. HTTP semantic conventions would have all HTTP libraries create a http.client.duration ValueRecorder. However, each library creates the instrument on its own Meter instance, so it is not problematic. Those Meters' name + version turn them into separate InstrumentationLibraryMetrics in OTLP.

@hdost
Copy link

hdost commented Feb 13, 2021

@aabmass yes I think we're on mostly the same page there. Over all yes even in the 3P scenario since they are the same instrument type they would succeed.
How ever getting a Meter with a different name should be based on the instrumentation library not the instrumented library.

So if I have some name space com.mylibrary I wouldn't call MeterProvider.GetMeter("com.my library") I would call MeterProvider.GetMeter("io.opentelemetry.ask") or something similar.

@victlu
Copy link
Contributor Author

victlu commented Feb 16, 2021

DNS Name?

The OTLP proto spec says...

// name of the metric, including its DNS name prefix. It must be unique.

What is this DNS name? How is it constructed from...

  1. MeterProvider interface?
  2. GetMeter(name1, version1)
  3. CreateCounter(name2)

Monotonic Type?

The OTLP proto has the following data types...

IntGauge, DoubleGauge, IntSum, DoubelSum, etc... spec

How do we distingush Counter vs UpDownCounter? Both would be IntSum/DoubleSum.

Why do we need Int/Double types?

Do we need to distingush Int vs Double for a Counter? (i.e. In User's Dashboards/Reports, would we show two different charts? One for Int vs one for Double for the same Named metric?)

Semantically speaking a Counter is a numeric, and implementation wise, we can add Ints and Doubles. Thus, IMHO, this is an internal optimization issue, and I could image the API/SDK optimally convert/promote internal operations/storage of shorts, ints, floats, doubles, (maybe even string formats), etc... as it sees fit.

What is Instrumentation vs Instrumented code?

Can we clarify what is the differences here? If I have Code A, and I added code to GetMeter(???) and meter.Add() into it. Would Code A be the instrumented code? Or the instrumentation code? What would I pass as the library name into GetMeter() in this case?

What if my Code A is packaged and used by other "Code A" like code?

I would think the name passed into GetMeter() would be the fully qualified class/package name where GetMeter() code is compiled into.

CreateCounter should always be it's own instance

  • Meter implementations MUST return an error when multiple instruments are registered by the same name.`

vs.

  • Meter.GetInstrument() called with same Identity will return reference to existing instrument.

Is this an API issue, or is this a SDK/Exporter/OTLP issue based on user's configuration?

I propose GetInstrument() always return a new instance  regardless of it's *Identity* equality.

It is up to the developer to share instances if it is desired. But this works only if the developer has control over the scope of the required code base.

When we talk about independently developed code/library, then, I think we need to rely on merging of equal "identity" in the SDK's aggregation/exporter/OTLP layer.

When we talk about merging different kind of instruments, who is to say we cannot "merge" counter into Summary and into Histogram, or reversed? (I don't know how, but someone smarter than me can likely make a case for it). Thus, if we keep instruments it's own instance in the API, we can delay the merge issue to the SDK (where vendors can play a role). Either way, we have this same question open about OTLP and Collectors (merging from multiple hosts) as well.

@aabmass
Copy link
Member

aabmass commented Feb 22, 2021

// name of the metric, including its DNS name prefix. It must be unique.

What is this DNS name?

I have a feeling this originally came from the Google Cloud Monitoring API here, maybe through OpenCensus. Cloud Monitoring uses domain names in metric names e.g. agent.googleapis.com/agent/api_request_count. OTel doesn't care if you do this, I think the comment should probably be removed.

How do we distingush Counter vs UpDownCounter? Both would be IntSum/DoubleSum.

From the labels on this issue, are we scoping this discussion only to the metrics API? From the API perspective, I can't see a good reason to have a Counter and UpDownCounter (or any other pair of instruments) with the same name. @victlu do you see a good use case for this?

Do we need to distingush Int vs Double for a Counter? (i.e. In User's Dashboards/Reports, would we show two different charts? One for Int vs one for Double for the same Named metric?)

I think this depends on the vendor system? From an API perspective, it makes sense to me to only allow a single instrument identity with a given name to avoid this problem

Can we clarify what is the differences here? If I have Code A, and I added code to GetMeter(???) and meter.Add() into it. Would Code A be the instrumented code? Or the instrumentation code? What would I pass as the library name into GetMeter() in this case?

They would be the same in this example, which is ok. They are only different when the instrumentation is provided as a separate library.

I would think the name passed into GetMeter() would be the fully qualified class/package name where GetMeter() code is compiled into.

+1, that is what we recommend in Python by passing __name__. Not sure about others

When we talk about independently developed code/library, then, I think we need to rely on merging of equal "identity" in the SDK's aggregation/exporter/OTLP layer.

That makes sense to me, and I think it would be a simple change based on the SDK spec we had (or it might already be supported?). In a way though, this provides an way for unrelated packages to write to each other's instruments? Kinda feels like violating private/public visibility a bit?

As for OTLP, I haven't heard a concrete plan to implement merging in the collector (if that's what you mean) because of the difficulty of the problem. Depending on the backend, couldn't you already configure merging like this from different resources/label sets/instrumentation libraries (e.g. prometheus recording rules)?

When we talk about merging different kind of instruments, who is to say we cannot "merge" counter into Summary and into Histogram, or reversed?

You might find this OTEP for "Views" interesting, which allows merging different instruments https://github.com/open-telemetry/oteps/blob/main/text/metrics/0126-Configurable-Metric-Aggregations.md

@victlu
Copy link
Contributor Author

victlu commented Feb 24, 2021

To consolidate this conversation, if I say below, would this be true today?

Identity (Fully Qualified Distinct Name) of a Metric

Available Fields

    var meterProvider = MetricProvider.Default;

    var meter = meterProvider.GetMeter(meter_name, meter_version);

    var instrument = meter.CreateInt64Counter(instrument_name);

    var labels = new KeyValuePair<string,string>[]
    {
        KeyValuePair.Create("DimName1", "DimValue1"),
        KeyValuePair.Create("DimName2", "DimValue2")
    };

    instrument.Add(default(SpanContext), value, labels);

Meter Name

  • User provided string

  • Recommend using fully qualified name of instrumenting class

e.g. io.opentelemetry.contrib.mongodb

Meter Version [Optional]

  • User provided string
  • Default to "" (?)
  • No validation (should we recommend semantic versioning?)

e.g. 1.0.0

Instrument Name

  • User provided string

  • Should adhere to Semantic Convention

    • They are non-empty strings
    • They are case-insensitive
    • The first character must be non-numeric, non-space, non-punctuation
    • Subsequent characters must belong to the alphanumeric characters, '_', '.', and '-'.

e.g. http_request_latency

Instrument Data Type

  • Inferred by Function Name

  • Enums:

    • Int64
    • Double

Instrument Kind

  • Inferred by Function Name

  • Enums:

    • Counter
    • UpDownCounter
    • ValueRecorder
    • SumObserver
    • UpDownSumObserver
    • ValueObserver

Identity of a Metric

Must include:

  • Meter Name
  • Meter Version
  • Instrument Kind
  • Instrument Data Type
  • Instrument Name

Other possible values:

  • MeterProvider instance?
  • Labels (Names and/or Values)
  • Unit
  • Aggregation Used
  • OTLP "types" (i.e. Gauge, Sum, Histogram, Summary). Is this maybe a Instrument Family?

List of topics to be discuss

  • Do we have a string representation? (i.e. OTLP name?)

    • What do we use for delimiters (e.g. "|")?

    e.g. io.opentelemetry.contrib.mongodb | 1.0.0 | Int64 | Counter | http_request_latency

  • Fully qualified Meter Name is language specific? Or we have semantic convention for it?

  • Who needs to have a unique metric name? (i.e. SDK for aggregation? Exporter? Collector for aggregation?)

  • How does Labels play into uniqueness of a Metric identity?

  • Do we need to include "named"/instance of the metric provider?

  • Why do we need Instrument Data Type to be part of Function Name?

  • Specs says we cannot include Meter Name and Meter Version as part of a metric's identity

    Each distinctly named Meter establishes a separate namespace for its metric instruments, making it possible for multiple instrumentation libraries to report the metrics with the same instrument name used by other libraries. The name of the Meter is explicitly not intended to be used as part of the instrument name, as that would prevent instrumentation libraries from capturing metrics by the same name.

@reyang reyang added area:data-model For issues related to data model and removed area:api Cross language API specification issue labels Mar 10, 2021
@reyang reyang assigned victlu and unassigned reyang Mar 10, 2021
@jsuereth
Copy link
Contributor

jsuereth commented Apr 2, 2021

This should be fixed on the DataModel side once #1574 is submitted, PTAL @victlu and let me know (identity of metric data stream is called out explicitly).

@victlu
Copy link
Contributor Author

victlu commented Apr 20, 2021

This issue can be closed, as the following spec changes have helped to clarify the "identity" of an instrument vs metrics.

@victlu victlu closed this as completed Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:data-model For issues related to data model priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

No branches or pull requests

7 participants