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

Rename "advice" to "advisory parameters" #3662

Merged
merged 2 commits into from
Aug 28, 2023

Conversation

dashpole
Copy link
Contributor

Fixes #3621

Changes

  • Change advice to advisory parameters. The intention is to change advice from implicitly being a concrete parameter to being a category of parameter.
  • Add advice/advisory parameters to the Asynchronous Instrument API (which I believe was an oversight). It is currently only part of the Synchronous instrument API.
  • Clarify that what makes "advisory parameters" different from other parameters is that implementations may ignore them.
  • Consistently use "parameters" instead of "fields" in the API

@jack-berg @jsuereth is this something like what you had in mind? Some alternative approaches we could consider if we want to keep "advice" as a noun:

  • Keep "advice" as a noun, but add flexible language e.g. "advice can be omitted in favor of the underlying parameters"
  • Keep "advice" as a noun, but describe it as a category of parameter.

@dashpole dashpole requested review from a team August 15, 2023 19:55
@dashpole dashpole force-pushed the advisory_parameters branch from 1e2f0e1 to c464a7c Compare August 15, 2023 19:56
@reyang
Copy link
Member

reyang commented Aug 15, 2023

  • Change advice to advisory parameters. The intention is to change advice from implicitly being a concrete parameter to being a category of parameter.

Why not using "hints" (which was the original name that we've been using during the discussion)? @jack-berg

  • Add advice/advisory parameters to the Asynchronous Instrument API (which I believe was an oversight). It is currently only part of the Synchronous instrument API.

+1

@MrAlias
Copy link
Contributor

MrAlias commented Aug 15, 2023

Why not using "hints" (which was the original name that we've been using during the discussion)? @jack-berg

It would need to be "hints parameters" still to indicate that these are a category of parameters, right?

@dashpole
Copy link
Contributor Author

dashpole commented Aug 15, 2023

Why not using "hints" (which was the original name that we've been using during the discussion)? @jack-berg

I don't think that would address #3621, as it is still a noun, which implicitly is a parameter. "hint parameters" would work, but I don't have a strong opinion on advice/advisory parameter vs hint/hint parameter

edit: tyler beat me to it

@reyang
Copy link
Member

reyang commented Aug 15, 2023

Why not using "hints" (which was the original name that we've been using during the discussion)? @jack-berg

It would need to be "hints parameters" still to indicate that these are a category of parameters, right?

I was thinking about SDK View https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#view.

The SDK MUST provide the means to register Views with a MeterProvider.

Whether Views are provided as a single argument or multiple arguments is up to the implementation. Hints can be handled using the same way.

@reyang
Copy link
Member

reyang commented Aug 15, 2023

Why not using "hints" (which was the original name that we've been using during the discussion)? @jack-berg

I don't think that would address #3621, as it is still a noun, which implicitly is a parameter. "hint parameters" would work, but I don't have a strong opinion on advice/advisory parameter vs hint/hint parameter

See my reply here #3662 (comment). Another thing to consider - do we want to stop folks from using a single parameter for all advice/hints? I don't see a strong reason why we want to have such preference.

@dashpole dashpole force-pushed the advisory_parameters branch from 35dc3ef to cc829ac Compare August 21, 2023 13:31
@jsuereth
Copy link
Contributor

The only piece of this I'm not sure whether or not it's missing is the bit @jack-berg / @MrAlias were discussing regarding having a firm description of which advice to accept in the event you have two registration events with conflicting advice (i.e. the whole first-seen-by-runtime verbage). You may have it in this PR and I missed it, or it's covered by some other section of the spec and I also failed to recall. Just want to make sure that's covered.

@dashpole
Copy link
Contributor Author

having a firm description of which advice to accept in the event you have two registration events with conflicting advic

That was done in #3661.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider allowing a simpler API for histogram default buckets
5 participants