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

Clarify MetricProducer "Produce() does not have any required parameters" #3601

Closed
damemi opened this issue Jul 14, 2023 · 7 comments · Fixed by #3612
Closed

Clarify MetricProducer "Produce() does not have any required parameters" #3601

damemi opened this issue Jul 14, 2023 · 7 comments · Fixed by #3612
Assignees
Labels
spec:metrics Related to the specification/metrics directory

Comments

@damemi
Copy link
Contributor

damemi commented Jul 14, 2023

What are you trying to achieve?

The MetricProducer spec indicates the following for the Produce() method:

Produce does not have any required parameters, however, OpenTelemetry SDK authors MAY choose to add parameters (e.g. timeout).

It is unclear whether this means that SDK authors may add required parameters, or if Produce() must not have any required parameters.

If the former, it should be clarified that "authors MAY choose to add required or optional parameters" (or something similar).

If the latter, it should be clarified that "authors MAY choose to add optional parameters".

This question arose in the Go metrics sdk for MetricProducer: open-telemetry/opentelemetry-go#3668. The Produce() function in Go has a required context.Context parameter.

Ref #3599

@dashpole
Copy link
Contributor

It is the former: authors MAY choose to add required or optional parameters.

@aabmass
Copy link
Member

aabmass commented Jul 19, 2023

Another point here, we have this requirement

If the batch of [Metric points](./data-model.md#metric-points) returned by
`Produce()` includes a [Resource](../resource/sdk.md), the `MetricProducer` MUST
accept configuration for the [Resource](../resource/sdk.md).

I take that as the Produce function must accept the Resource in this case. I think at least that requirement should be moved into the Produce() subsection

@dashpole
Copy link
Contributor

I take that as the Produce function must accept the Resource in this case.

The original intent was that Resource would be passed once when the MetricProducer is constructed, and then used to set the resource returned by all Produce() calls. We can make it more flexible if there is a design you think works better for JS.

@aabmass
Copy link
Member

aabmass commented Jul 20, 2023

  1. The MetricProducer is something that a user or library implements, so the MUST requirement is not actually a requirement for the SDK
  2. What should the MetricReader do when the Resource returned by a registered MetricProducer is different from the SDKs resource? In JS's case, the result of a collection is a single ResourceMetrics i.e. one Resource. So I could merge the Resources or only use the SDK's resource. I like the latter here but I don't think that complies with spec.

@dashpole
Copy link
Contributor

The MetricProducer is something that a user or library implements, so the MUST requirement is not actually a requirement for the SDK

That is correct. It is technically a requirement for the bridges. MetricProducer implementations SHOULD accept configuration for the AggregationTemporality of produced metrics. also is meant for bridges, rather than SDKs. The SDK spec probably isn't the right place for it to live, but there isn't a great alternative.

What should the MetricReader do when the Resource returned by a registered MetricProducer is different from the SDKs resource? In JS's case, the result of a collection is a single ResourceMetrics i.e. one Resource. So I could merge the Resources or only use the SDK's resource. I like the latter here but I don't think that complies with spec.

I think the spec is silent on the SDK's behavior if it gets a resource from the bridge. I think the latter is acceptable. We initially had more specific language saying that the MetricReader MUST override the resource from a producer with its own, but decided to drop it to leave that open to SDK authors.

@aabmass
Copy link
Member

aabmass commented Jul 20, 2023

Ok, so if I implement the SDK to drop the resource from a bridge, then

MetricProducer MUST accept configuration for the Resource.

seems unnecessary since I'll just throw it away. Maybe we can switch this to a SHOULD?


Also sorry I think we have gotten off topic here. I can open a separate issue if you agree David

@dashpole
Copy link
Contributor

I can open a separate issue if you agree David

I agree it should be SHOULD. Feel free to open a separate issue.

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.

4 participants