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

opentelemetry: feature-flag MetricsSubscriber #2234

Merged
merged 3 commits into from Jul 21, 2022
Merged

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Jul 20, 2022

In the upstream opentelemetry crate, the trace and metrics
features are gated by separate feature flags. This allows users who are
only using OpenTelemetry for tracing, or who are only using it for
metrics, to pick and choose what they depend on.

Currently, the release version of tracing-opentelemetry only provides
tracing functionality, and therefore, it only depends on opentelemetry
with the trace feature enabled. However, the metrics support added in
#2185 adds a dependency on the opentelemetry/metrics feature. This is
currently always enabled. We should probably follow the same approach as
upstream opentelemetry, and allow enabling/disabling metrics and
tracing separately.

This branch adds a metrics feature to tracing-opentelemetry, and
makes the MetricsSubscriber from #2185 gated on the metrics feature.
This feature flag is on by default, like the upstream
opentelemetry/metrics feature, but it can be disabled using
default-features = false.

We should probably do something similar for the tracing components of
the crate, and make them gated on a trace feature flag, but adding a
feature flag to released APIs is not semver-compatible, so we should
save that until the next breaking release.

In the upstream `opentelemetry` crate, the `trace` and `metrics`
features are gated by separate feature flags. This allows users who are
only using OpenTelemetry for tracing, or who are only using it for
metrics, to pick and choose what they depend on.

Currently, the release version of `tracing-opentelemetry` only provides
tracing functionality, and therefore, it only depends on `opentelemetry`
with the `trace` feature enabled. However, the metrics support added in
#2185 adds a dependency on the `opentelemetry/metrics` feature. This is
currently always enabled. We should probably follow the same approach as
upstream `opentelemetry`, and allow enabling/disabling metrics and
tracing separately.

This branch adds a `metrics` feature to `tracing-opentelemetry`, and
makes the `MetricsSubscriber` from #2185 gated on the `metrics` feature.
This feature flag is on by default, like the upstream
`opentelemetry/metrics` feature, but it can be disabled using
`default-features = false`.

We should probably do something similar for the tracing components of
the crate, and make them gated on a `trace` feature flag, but adding a
feature flag to released APIs is not semver-compatible, so we should
save that until the next breaking release.
@hawkw hawkw requested a review from bryangarza July 20, 2022 18:14
@hawkw hawkw requested review from jtescher and a team as code owners July 20, 2022 18:14
Copy link
Collaborator

@jtescher jtescher left a comment

Choose a reason for hiding this comment

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

Yep should be flagged. Metrics are still quite unstable as well, could consider something more ominous like metrics-unstable

@hawkw
Copy link
Member Author

hawkw commented Jul 20, 2022

@jtescher

Metrics are still quite unstable as well, could consider something more ominous like metrics-unstable

I considered that as well. In practice, though, I think the metrics API in tracing-opentelemetry specifically is not actually going to be all that unstable, even if the underlying OpenTelemetry metrics stuff changes a bunch. Unlike the trace APIs, the metrics API in tracing-opentelemetry doesn't actually expose anything from the opentelemetry crate, so even if opentelemetry changes, the tracing-opentelemetry metrics iinterfaces won't change. And the API surface area is currently quite small: it exposes a Subscriber type and a constructor, basically. Any breaking changes in opentelemetry would be hidden behind that minimal API surface.

If we end up adding a bunch of configuration options for metrics later, or something, though, it might make sense to mark those APIs as unstable, especially if they depend on types from the opentelemetry crate in their public APIs, or configure features that are unstable in the OpenTelemetry specification?

What do you think?

@jtescher
Copy link
Collaborator

I considered that as well. In practice, though, I think the metrics API in tracing-opentelemetry specifically is not actually going to be all that unstable, even if the underlying OpenTelemetry metrics stuff changes a bunch.

Yeah that's a fair point, the setup to construct a meter to use in the metrics layer will be quite unstable, but the impl in this crate may not expose the same instability. I'm fine with the current naming.

@hawkw hawkw enabled auto-merge (squash) July 21, 2022 18:13
@bryangarza
Copy link
Member

Looks good to me!

@hawkw hawkw merged commit cd81de4 into master Jul 21, 2022
@hawkw hawkw deleted the eliza/metrics-feature branch July 21, 2022 22:46
hawkw added a commit that referenced this pull request Jul 29, 2022
In the upstream `opentelemetry` crate, the `trace` and `metrics`
features are gated by separate feature flags. This allows users who are
only using OpenTelemetry for tracing, or who are only using it for
metrics, to pick and choose what they depend on.

Currently, the release version of `tracing-opentelemetry` only provides
tracing functionality, and therefore, it only depends on `opentelemetry`
with the `trace` feature enabled. However, the metrics support added in
#2185 adds a dependency on the `opentelemetry/metrics` feature. This is
currently always enabled. We should probably follow the same approach as
upstream `opentelemetry`, and allow enabling/disabling metrics and
tracing separately.

This branch adds a `metrics` feature to `tracing-opentelemetry`, and
makes the `MetricsLayer` from #2185 gated on the `metrics` feature.
This feature flag is on by default, like the upstream
`opentelemetry/metrics` feature, but it can be disabled using
`default-features = false`.

We should probably do something similar for the tracing components of
the crate, and make them gated on a `trace` feature flag, but adding a
feature flag to released APIs is not semver-compatible, so we should
save that until the next breaking release.
@hawkw hawkw mentioned this pull request Jul 29, 2022
hawkw added a commit that referenced this pull request Jul 29, 2022
In the upstream `opentelemetry` crate, the `trace` and `metrics`
features are gated by separate feature flags. This allows users who are
only using OpenTelemetry for tracing, or who are only using it for
metrics, to pick and choose what they depend on.

Currently, the release version of `tracing-opentelemetry` only provides
tracing functionality, and therefore, it only depends on `opentelemetry`
with the `trace` feature enabled. However, the metrics support added in
#2185 adds a dependency on the `opentelemetry/metrics` feature. This is
currently always enabled. We should probably follow the same approach as
upstream `opentelemetry`, and allow enabling/disabling metrics and
tracing separately.

This branch adds a `metrics` feature to `tracing-opentelemetry`, and
makes the `MetricsLayer` from #2185 gated on the `metrics` feature.
This feature flag is on by default, like the upstream
`opentelemetry/metrics` feature, but it can be disabled using
`default-features = false`.

We should probably do something similar for the tracing components of
the crate, and make them gated on a `trace` feature flag, but adding a
feature flag to released APIs is not semver-compatible, so we should
save that until the next breaking release.
@jtescher jtescher added the crate/opentelemetry Related to the `tracing-opentelemetry` crate. label Sep 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/opentelemetry Related to the `tracing-opentelemetry` crate.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants