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.

Sign up for GitHub

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

Jump to bottom

Move specification for implementations of the metric API into own specification #3171

Merged
merged 24 commits into from
Feb 14, 2023
Merged

Move specification for implementations of the metric API into own specification #3171

merged 24 commits into from
Feb 14, 2023

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Feb 2, 2023

Fixes #3071
Fixes #3072

Changes

  • Move metric SDK specification that currently exists in the API to sdk.md
  • Remove generic specification for implementations of the metric API from api.md
    • All OTel implementations are explicitly defined with the addition of the No-Op implementation
  • Add noop.md to defined the metric No-Op implementation of the metric API

In the reshuffle, this clarified ...

Outside of these clarifications, all changes should be editorial.

@MrAlias MrAlias added the spec:metrics Related to the specification/metrics directory label Feb 2, 2023
@MrAlias MrAlias requested review from a team February 2, 2023 17:26
This was referenced Feb 2, 2023
Merged
Closed
@MrAlias MrAlias mentioned this pull request Feb 2, 2023
Closed
@arminru arminru added area:api Cross language API specification issue area:sdk Related to the SDK labels Feb 7, 2023
jsuereth
jsuereth approved these changes Feb 9, 2023
View reviewed changes
Copy link
Contributor

@jsuereth jsuereth left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm very supportive of these changes. I think having the behavior you're moving out of the API specification caused some confusion and implementation friction, particularly for python.

dashpole
dashpole approved these changes Feb 9, 2023
View reviewed changes
Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

This looks great! Just nits

specification/metrics/api.md Outdated Show resolved Hide resolved
specification/metrics/noop.md Outdated Show resolved Hide resolved
specification/metrics/noop.md Outdated Show resolved Hide resolved
specification/metrics/noop.md Outdated Show resolved Hide resolved
specification/metrics/noop.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
@MrAlias @dashpole
Apply suggestions from code review
71e52f2 
Co-authored-by: David Ashpole <dashpole@google.com>
Aneurysm9
Aneurysm9 reviewed Feb 9, 2023
View reviewed changes
specification/metrics/noop.md Outdated Show resolved Hide resolved
specification/metrics/noop.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
jack-berg
jack-berg approved these changes Feb 13, 2023
View reviewed changes
Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Couple minor comments but looks good. Thanks!

pellared
pellared approved these changes Feb 14, 2023
View reviewed changes
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

I have read it twice. Nice improvement.

@reyang reyang merged commit 545d25b into open-telemetry:main Feb 14, 2023
@reyang
Copy link
Member

reyang commented Feb 14, 2023

Merged. Follow up question - do we think tracing and logging spec should do the same?

@MrAlias MrAlias deleted the noop-metric branch February 14, 2023 19:35
@MrAlias
Copy link
Contributor Author

MrAlias commented Feb 14, 2023

Merged. Follow up question - do we think tracing and logging spec should do the same?

I think so. We should probably open an issue to track this work.

@MrAlias MrAlias mentioned this pull request Feb 14, 2023
Open
@MrAlias
Copy link
Contributor Author

MrAlias commented Feb 14, 2023

Merged. Follow up question - do we think tracing and logging spec should do the same?

I think so. We should probably open an issue to track this work.

@reyang I can open an issue unless you have already started one. Let me know.

@reyang
Copy link
Member

reyang commented Feb 14, 2023

@reyang I can open an issue unless you have already started one. Let me know.

I haven't, please go ahead.
I was thinking maybe it's less a concern for tracing because most SIGs are already done with the stable release.

@jack-berg jack-berg mentioned this pull request Mar 2, 2023
Merged
jack-berg added a commit that referenced this pull request Mar 27, 2023
@jack-berg
Ensure logs bridge API doesn't contain implementation details (#3275)
feabaab 
Contributes to #3268.

@MrAlias did some good work in the metrics API / SDK recently in #3171
and #3067 to ensure that the metrics API spec doesn't contain SDK
implementation details.

This PR adopts similar language in the Logs Bridge API / SDK documents,
which includes breaking out a `noop.md` document.
@jack-berg jack-berg mentioned this pull request Aug 2, 2023
Closed
This was referenced Feb 15, 2024
Merged
Merged
Merged
carlosalberto pushed a commit that referenced this pull request Feb 23, 2024
@MrAlias
[editorial] Remove implementation specific specification from metric …
94fabf7 
…API (#3890)

The api.md document specifies the Metric API, it does not defined the
OpenTelemetry defined implementations of this API. Remove or correct the
details within this API and leave it for the implementation
specification to define.

Importantly, the API is decoupled from implementation for a reason.
Third-party implementation may exists that OTel does not have domain
over. This document needs to written with that in mind.

Related to
#3171
and
#3071.
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@MrAlias
Move specification for implementations of the metric API into own spe…
a8fe46a 
…cification (open-telemetry#3171)
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@jack-berg
Ensure logs bridge API doesn't contain implementation details (open-t…
58063e7 
…elemetry#3275)

Contributes to open-telemetry#3268.

@MrAlias did some good work in the metrics API / SDK recently in open-telemetry#3171
and open-telemetry#3067 to ensure that the metrics API spec doesn't contain SDK
implementation details.

This PR adopts similar language in the Logs Bridge API / SDK documents,
which includes breaking out a `noop.md` document.
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@MrAlias
[editorial] Remove implementation specific specification from metric …
35f6fd5 
…API (open-telemetry#3890)

The api.md document specifies the Metric API, it does not defined the
OpenTelemetry defined implementations of this API. Remove or correct the
details within this API and leave it for the implementation
specification to define.

Importantly, the API is decoupled from implementation for a reason.
Third-party implementation may exists that OTel does not have domain
over. This document needs to written with that in mind.

Related to
open-telemetry#3171
and
open-telemetry#3071.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jack-berg jack-berg jack-berg approved these changes

@Aneurysm9 Aneurysm9 Aneurysm9 left review comments

@tsloughter tsloughter tsloughter left review comments

@reyang reyang reyang approved these changes

@pellared pellared pellared approved these changes

@dashpole dashpole dashpole approved these changes

@jsuereth jsuereth jsuereth approved these changes

Assignees

@jsuereth jsuereth

Labels
area:api Cross language API specification issue area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory
Projects
None yet
Milestone
No milestone
9 participants
@MrAlias @reyang @jsuereth @tsloughter @Aneurysm9 @dashpole @pellared @jack-berg @arminru