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

Support measurement processors in Metrics SDK #4298

Open
Blinkuu opened this issue Nov 18, 2024 · 8 comments · May be fixed by #4318
Open

Support measurement processors in Metrics SDK #4298

Blinkuu opened this issue Nov 18, 2024 · 8 comments · May be fixed by #4318
Assignees
Labels
spec:metrics Related to the specification/metrics directory triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned

Comments

@Blinkuu
Copy link

Blinkuu commented Nov 18, 2024

What are you trying to achieve?

I would like to be able to enrich my telemetry using members encoded inside the Baggage header. The idea is to enable an "end-to-end telemetry correlation" use case by leveraging Context and Baggage.

This is easy to do using the Trace SDK by leveraging the SpanProcessor. Concrete implementations exist, such as the one for Go.

Furthermore, a similar concept exists in the Logs SDK - the LogRecordProcessor. Similarly, there seem to exist concrete implementations.

What did you expect to see?

According to the Design Goals of Metrics Specification, enriching metrics attributes via Baggage and Context is a top priority:

Being able to connect metrics to other signals. For example, metrics and traces can be correlated via exemplars, and metrics attributes can be enriched via Baggage and Context. Additionally, Resource can be applied to logs/metrics/traces in a consistent way.

Based on the quote above, I expected to see a similar "processor" concept for Metrics SDK, but that doesn't seem to be the case.

Additional context.

Even though the "processor" concept isn't fully materialized, some evidence exists in the form of implementation, namely in the opentelemetry-java project. It looks like Views can accept a set of AttributesProcessors (see here). Moreover, there even exists an implementation of a processor that extracts baggage members and attaches them as attributes (see here). Nevertheless, this feature is marked as experimental.

Related Issues.

@Blinkuu Blinkuu added the spec:metrics Related to the specification/metrics directory label Nov 18, 2024
@cijothomas
Copy link
Member

#1938 for reference.

@pellared
Copy link
Member

#1938 for reference.

From the SIG meeting notes:

View already covered most of the features, if we need something else, we should first look at how to improve View rather than introducing another concept at this moment.

@pellared
Copy link
Member

pellared commented Nov 19, 2024

By @jmacd in #4256 (comment):

In the Lightstep metrics SDK, we have added a MeasurementProcessor API which has been discussed in the OpenTelemetry specification as far back as the OpenCensus origin. I would like to see OTel add this feature!

MeasurementProcessor absolutely uses the context to derive metric attributes. However, to conditionally enable or disable a metric instrument based on context would introduce an undetectable bias in the data. For example, if you only enable a metric instrument when the context is traced, then the metric instrument will under-count depending on sampling rate and the consumer of the data will have no way of knowing this. Metric instruments should strive to measure everything--users should let the context be used to adjust which attributes are used, not to conditionalize.

@jmacd, do you want to sponsor this or maybe even work on this or help someone who would like to address the issue?

@bogdandrutu
Copy link
Member

I think, we could consider also to allow views to read from Baggage to determine attributes.

@jack-berg
Copy link
Member

I like the measurement processor concept and am in favor of seeing it added. In addition to adding attributes from baggage, it would allow you to:

  • Filter out values that are outside of acceptable range.
  • Enrich measurements with other bits of data stored in context.
  • Perfect lossless unit translation. Unit translation of aggregated explicit bucket and exponential histogram aggregation is lossy.
  • Pipe raw measurements directly out of process.

There are definitely some performance implications, since changing (adding or removing) attributes on the hotpath is not free from a CPU or memory perspective. But users would be opting into this behavior and can make a decision on whether the behavior is worth the performance penalty.

@trask trask added the triage:accepted:needs-sponsor Ready to be implemented, but does not yet have a specification sponsor label Nov 19, 2024
@jsuereth
Copy link
Contributor

I think, we could consider also to allow views to read from Baggage to determine attributes.

This was in the specification at one point, perhaps it was removed?

@cijothomas
Copy link
Member

I think, we could consider also to allow views to read from Baggage to determine attributes.

This was in the specification at one point, perhaps it was removed?

yes. https://github.com/open-telemetry/opentelemetry-specification/pull/1730/files#diff-32d5f289beed96900fa1e6e69a3ee9cc0b6503ce8883672011c44bc772c0fa2aR126

@reyang
Copy link
Member

reyang commented Nov 26, 2024

I can be the sponsor.

@reyang reyang added triage:accepted:ready Ready to be implemented. Small enough or uncontroversial enough to be implemented without sponsor and removed triage:accepted:needs-sponsor Ready to be implemented, but does not yet have a specification sponsor labels Nov 26, 2024
@pellared pellared added triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned and removed triage:accepted:ready Ready to be implemented. Small enough or uncontroversial enough to be implemented without sponsor labels Nov 26, 2024
Blinkuu added a commit to Blinkuu/opentelemetry-specification that referenced this issue Dec 3, 2024
@Blinkuu Blinkuu linked a pull request Dec 3, 2024 that will close this issue
5 tasks
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 triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

8 participants