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

Consider making Advice non-identifying for instruments #3622

Closed
dashpole opened this issue Jul 24, 2023 · 9 comments · Fixed by #3661
Closed

Consider making Advice non-identifying for instruments #3622

dashpole opened this issue Jul 24, 2023 · 9 comments · Fixed by #3661
Assignees
Labels
spec:metrics Related to the specification/metrics directory

Comments

@dashpole
Copy link
Contributor

During the implementation of a Golang prototype of the Advice API, we found the requirement that Advice be identifying for instruments to be odd: open-telemetry/opentelemetry-go#4162 (comment)

In the current Instrument Advice API, it says:

Each Instrument will have the following fields:

  • The name of the Instrument
  • The kind of the Instrument - whether it is a Counter or one of the other kinds, whether it is synchronous or asynchronous
  • An optional unit of measure
  • An optional description
  • Optional advice (experimental)

Instruments are associated with the Meter during creation. Instruments are identified by all of these fields.

If Advice is identifying, it means two histograms can be created that have identical names, units, and descriptions, but different default bucket boundaries. This seems to produce some odd behavior: What happens if I change the buckets, or switch to an exponential histogram using a View? Will the two instruments produce the same data?

I think we should change Advice to be non-identifying for instruments.

Side note: I also think description should not be identifying, but it appears that ship has sailed.

cc @MrAlias @jack-berg

@dashpole dashpole added the spec:metrics Related to the specification/metrics directory label Jul 24, 2023
@jmacd
Copy link
Contributor

jmacd commented Jul 27, 2023

I agree. Advice is non-identifying.

@jack-berg
Copy link
Member

I agree.

Just so we're clear, my interpretation is that because advice is non-identifying, two histograms with the same identity and different advices will result in 1 metric stream. And thus we have to pick one set of advice to apply to the resulting metric stream, which should naturally be the first we see.

@MrAlias
Copy link
Contributor

MrAlias commented Jul 27, 2023

which should naturally be the first we see

Can we be sure to make that clear and normative in whatever fix is applied for those of us that aren't as intuitive (😀)?

@MrAlias
Copy link
Contributor

MrAlias commented Aug 9, 2023

I was looking at how to update the spec for this, but ran into the following question:

When resolving two instruments with identical fields except for the advice, doesn't the fact that there is a conflict to resolve mean that the advice is indeed identifying?

@dashpole
Copy link
Contributor Author

dashpole commented Aug 9, 2023

When resolving two instruments with identical fields except for the advice, doesn't the fact that there is a conflict to resolve mean that the advice is indeed identifying?

I've been interpreting "identifying" to mean that different inputs to the parameter produce different instruments, and "non-identifying" to mean that different inputs to the parameter produces the same instrument. Similar to the case-sensitive vs case-insensitive debate, I would consider the (case insensitive) name to be identifying, but the case of letters in the name to be non-identifying. I think a first-advice-wins solution would work here as it did for the case sensitivity of the name.

@jack-berg
Copy link
Member

jack-berg commented Aug 9, 2023

identical fields except for the advice

Here's my understanding:

  • Instruments are identical when name (case insensitive), kind, unit, and description are equal. Identical means measurements contribute to the same metric stream.
  • If the names are the same, but there is some difference in one of the identifying fields, we have an identity conflict and produce different metric streams. Log a warning.
  • If all identifying fields are equal, but there is some difference in a non-identifying field like advice, then we produce a single metric stream using the non-identifying fields of the first seen. TBD on logging a warning, but probably yes.

@MrAlias
Copy link
Contributor

MrAlias commented Aug 9, 2023

  • If all identifying fields are equal, but there is some difference in a non-identifying field like advice, then we produce a single metric stream using the non-identifying fields of the first seen.

Does this mean no warning should be logged?

@jack-berg
Copy link
Member

Does this mean no warning should be logged?

Sorry - didn't include logging behavior in that. Updated.

@dashpole
Copy link
Contributor Author

I'll work on a PR to address this. Notes from the sig meeting:

  • We need to make sure we don't make statements about how languages are initialized. "first-seen" might be hard to define in some languages.

jack-berg pushed a commit that referenced this issue Aug 16, 2023
Fixes
#3622

The language for resolving conflicts matches the language used for
naming conflicts.

cc @MrAlias @jack-berg @jmacd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants