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

Metric SDK specification OUTLINE #347

Merged
merged 36 commits into from
Aug 20, 2020

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Nov 12, 2019

This outlines the components of the current OTel-Go Metric SDK. This is an outline to facilitate breaking the task of writing this specification into smaller tasks.

Part of #626.
Resolves #659.

@arminru
Copy link
Member

arminru commented Nov 12, 2019

The metrics SDK spec should also define how Meter name and version (see Named Tracers and Meters OTEP) should be used and exposed as it does for named tracers here:
https://github.com/open-telemetry/opentelemetry-specification/pull/264/files#diff-30f6673fb402c86fc97f1337f40e4c87R11-R18

New `Tracer` instances are always created through a `TracerFactory` (see [API](api-tracing.md#obtaining-a-tracer)).
The `name` and `version` arguments supplied to the `TracerFactory` must be used
to create a `Resource` instance which is stored on the created `Tracer`.
All configuration objects (SDK specific) and extension points (span processors,
propagators) must be provided to the `TracerFactory`. `Tracer` instances must
not duplicate this data (unless for read-only access) to avoid that different
`Tracer` instances of a `TracerFactory` have different versions of these data.
The readable representations of all `Span` instances created by a `Tracer` must
provide a `getLibraryResource` method that returns this `Resource` information
held by the `Tracer`.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice writeup. I left a few comments asking for clarification.

specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@jmacd jmacd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tigrannajaryan.

specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor Author

jmacd commented Nov 12, 2019

@arminru Just to acknowledge your point, I'll say that the current Go SDK does not include any behavior related to named Meters. It implements the provider pattern to get a named Meter, but it currently does not use the name. This is pending work and I intended for this document to track that work, so I haven't included anything about named meters in this document, but you're right that it belongs here.

OTOH, the user-facing document does describe the behavior that is expected for named Meters, see here: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-metrics-user.md#metric-names

This requirement is one of the reasons I'm alarmed by the confusion surrounding Named Tracers. If we did not have a "named Meter" concept already in the specification, the metrics API specification has to be extended with some notion of a namespace for metrics, I think.

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid addition. I left a few comments for small fixes and wondering if we can specify requirements a bit more concretely.

specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial Review

specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor Author

jmacd commented Nov 13, 2019

@freeformzSFDC I recommend reviewing the two API specification documents before reviewing this document. See api-metrics.md and api-metrics-user.md. I will try to clarify all the points you raised (tomorrow). Thanks.

@arminru
Copy link
Member

arminru commented Nov 13, 2019

This requirement is one of the reasons I'm alarmed by the confusion surrounding Named Tracers. If we did not have a "named Meter" concept already in the specification, the metrics API specification has to be extended with some notion of a namespace for metrics, I think.

@jmacd I'm not sure if the name of a named meter (i.e., the name of the instrumentation library) is really suitable for namespacing. Should we consider adding a separate namespace parameter? Named meters (tracers) were initially intended for configuration, filtering and troubleshooting.

specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
specification/sdk-metric.md Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Nov 13, 2019

@jmacd I read most of those docs.

@jmacd
Copy link
Contributor Author

jmacd commented Nov 13, 2019

@MrAlias I've strengthened the requirement statements for aggregators (401292b) and addressed the typos you pointed out (6990089). Thanks and let me know how you like the new text.

@jmacd
Copy link
Contributor Author

jmacd commented Nov 13, 2019

@arminru I see your point. I have mixed feelings and am not sure what's the right specification. On the one hand, a named meter can act as a namespace without literally applying the library's name to the metric name in any way. This would imply that two named Meters cannot possibly define the same metric instrument. If two distinctly-named meters defined metrics with the same name, they would be assigned different Descriptors, and the exporter would be left to deal with a name conflict. The solution I had in mind (which I think you're opposed to) is that each descriptor would retain a reference to its Meter's name, and then exporters would be free to apply the namespace in the exporter-appropriate way. (Some systems use '.', some use '_', etc.)

On the other hand, we could support an explicit WithNamespace() option. This would need a new OTEP to be written. This is pretty standard, e.g., https://godoc.org/github.com/DataDog/datadog-go/statsd#WithNamespace, https://godoc.org/github.com/segmentio/stats#WithPrefix, https://godoc.org/github.com/uber-go/tally#ScopeOptions

@github-actions github-actions bot removed the Stale label Aug 14, 2020
@jmacd jmacd requested review from a team as code owners August 19, 2020 07:41
@jmacd jmacd changed the title WIP: Metric SDK specification Metric SDK specification OUTLINE Aug 19, 2020
@jmacd
Copy link
Contributor Author

jmacd commented Aug 19, 2020

@open-telemetry/specs-metrics-approvers As discussed at last week's SIG I have replaced the old draft with an outline. I also wrote a list of terms that will be used.

Copy link
Contributor

@jkwatson jkwatson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the outline + terminology looks like a good start. 👍

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The outline looks good.
Left a comment about a possible mismatch between .md file and .png file.

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great first step 👍

specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor Author

jmacd commented Aug 20, 2020

@bogdandrutu Please approve and merge.

GA Spec Burndown automation moved this from Required for GA, in progress to Required for GA, approved Aug 20, 2020
@bogdandrutu bogdandrutu merged commit 320b6fc into open-telemetry:master Aug 20, 2020
GA Spec Burndown automation moved this from Required for GA, approved to Required for GA, done Aug 20, 2020
TuckTuckFloof pushed a commit to TuckTuckFloof/opentelemetry-specification that referenced this pull request Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK 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
No open projects
GA Burndown
  
Required for GA; add release:required...
GA Spec Burndown
  
Required/Allowed for GA, resolved.
Development

Successfully merging this pull request may close these issues.

Metrics terminology: Rename Integrator -> Processor