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

Remove No-Op instrument and Meter creation restrictions #3322

Merged
merged 11 commits into from
Apr 4, 2023

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Mar 17, 2023

The current No-Op requires all instruments to be created from a Meter and a Meter to be created from a MeterProvider. This was inherited from the SDK specification, but it is not necessary. The No-Op performs no operations and hold no state, therefore creating these objects directly should not have logical or processing consequences. If there are consequences of doing this in a language implementation, they are still permitted to keep this restriction.

This change is desired by the Go SIG. We want to export these No-Op types so users can create them directly. This will allow them to embed these No-Op types in their SDKs to define default behaviour, and allow them to use them directly for testing (instead of having to go through the creation chain all starting with a MeterProvider).

The current No-Op requires all instruments to be created from a Meter
and a Meter to be created from a MeterProvider. This was inherited from
the SDK specification, but it is not necessary. The No-Op performs no
operations and hold no state, therefore creating these objects directly
will not have logical or processing consequences.

This change is desired by the Go SIG. We want to export these No-Op
types so users can create them directly. This will allow them to embed
these No-Op types in their SDKs to define default behaviour, and allow
them to use them directly for testing (instead of having to go through
the creation chain all starting with a MeterProvider).
@MrAlias MrAlias added the spec:metrics Related to the specification/metrics directory label Mar 17, 2023
@MrAlias MrAlias requested review from a team March 17, 2023 20:33
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.

Java forces users to go the meter provider -> meter -> instrument chain, and we'll probably stick to that for now, but it seems like a reasonable request to be able to jump to instruments directly.

@yurishkuro
Copy link
Member

This change is desired by the Go SIG. We want to export these No-Op types so users can create them directly.

I don't understand the motivation. Code can always do new(NoopProvider).createMeter().createInstrument() without changing the spec.

@Aneurysm9
Copy link
Member

The types need to be exported to be able to be embedded in SDK implementations. If they are exported they can be created directly. If they can be created directly then the implementation is not spec compliant.

@yurishkuro
Copy link
Member

Only NoopProvider needs to be exported in my example, not the individual instruments.

@Aneurysm9
Copy link
Member

The issue is not with reference to instances of the type, but the type identifier itself. To embed a type it must be referenced where it is to be embedded. To reference a type outside of the package where it is declared it must be exported.

type MyCounter struct {
    noop.Int64Counter
    // other fields
}

No instances of the type are involved and being able to create an instance of a noop.int64Counter from a noop.meterProvider doesn't matter when I need to reference noop.int64Counter and cannot unless it is noop.Int64Counter.

CHANGELOG.md Outdated Show resolved Hide resolved
@arminru arminru added the area:api Cross language API specification issue label Mar 27, 2023
@MrAlias
Copy link
Contributor Author

MrAlias commented Apr 3, 2023

@carlosalberto can we include this in the upcoming release?

@jmacd
Copy link
Contributor

jmacd commented Apr 3, 2023

markdown-link-check :(

@MrAlias
Copy link
Contributor Author

MrAlias commented Apr 3, 2023

markdown-link-check :(

That failure doesn't look related to these changes. It looks like another PR is needed to address the failure in ./specification/common/attribute-type-mapping.md.

@jmacd jmacd merged commit 6ba4a82 into open-telemetry:main Apr 4, 2023
@MrAlias MrAlias deleted the metric-noop-create-alt branch April 4, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants