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

Single-shot metrics #3105

Open
1 of 2 tasks
davispuh opened this issue Jul 21, 2022 · 13 comments
Open
1 of 2 tasks

Single-shot metrics #3105

davispuh opened this issue Jul 21, 2022 · 13 comments
Labels
never-stale signal:metrics Issues and PRs related to general metrics signal

Comments

@davispuh
Copy link

  • This only affects the JavaScript OpenTelemetry library
  • This may affect other libraries, but I would like to get opinions here first

I've been looking into how to export single-shot metrics but don't really see how. The issue with PeriodicExportingMetricReader is that it keeps sending metrics with same value even if there's no new records.
For example consider a use case where I want to emit timestamp each time user clicks mouse button. I wouldn't want that it's sent if user didn't made any clicks. But using PeriodicExportingMetricReader that's not possible.

meterProvider.addMetricReader(new PeriodicExportingMetricReader({
        exporter: new OTLPMetricExporter({
            url: 'https://otlp/v1/metrics'
        }),
}));

meterProvider.getMeter('default').createHistogram(metricName).record(clickTimestamp);

There are 2 different things:

  1. when the metric is recorded/sampled - eg. measure every 10 seconds
  2. when metrics is sent to endpoint - eg. every 1min send all collected metrics to endpoint

and it seems current implementation does both at same time but there should be way to do 1. point manually that would allow for single-shot metrics.

@legendecas
Copy link
Member

legendecas commented Jul 21, 2022

Have you considered AggregationTemporality.DELTA? With that configuration, the exported metric aggregation will no longer be exported again. It can be configured with the constructor parameter OTLPMetricExporterOptions of OTLPMetricExporter, like:

new OTLPMetricExporter({
  url: 'https://otlp/v1/metrics'
  temporalityPreference: AggregationTemporality.DELTA,
});

Hope this helps.

@davispuh
Copy link
Author

davispuh commented Jul 22, 2022

It solves the issue that same data isn't resent again but it still keeps sending metrics all the time to the endpont with empty dataPoints

{
  name: "...",
  "histogram": {
    "aggregationTemporality": 1,
    "dataPoints": []
  }
}

To me it looks like there is need for MetricReader that sends out metrics only when they become available and not sending all the time when there haven't been any new metrics emitted.

@legendecas
Copy link
Member

We have an issue tracking the ability to forget unused attributes: #2997. But there is no method to unregister an instrument yet -- as creating a new instrument is not as trivial as recording a metric event is, I would not recommend doing that in a repetitive way.

@davispuh
Copy link
Author

I don't think that would be good way - add metric, send it, then remove it. Especially it doesn't make sense to remove it when you know you'll send it again later just not periodically.

@legendecas
Copy link
Member

Do you suggest that we should not forget unused attributes or unregister instruments?

@dyladan
Copy link
Member

dyladan commented Jul 25, 2022

I think he's just saying that he wants to be able to export a metric only when a value is provided without unregistering. Right now even with delta temporality we export an empty metric on each export interval. One option might e to detect when this is happening and simply not export which wouldn't require unregistering.

@jmacd
Copy link

jmacd commented Jul 27, 2022

The OTLP exporter is welcome to skip exporting metrics with no data points, I feel. Does this need to be specified? I also feel that Delta temporality is the correct solution to ensure the desired behavior.

Lastly, in the Otel-Go API we avoided the verb "create" so that the expression for a single-shot metric would look more natural, e.g.,

meter.SyncFloat64().Histogram("histo").Record(ctx, 100)

@legendecas
Copy link
Member

legendecas commented Jul 28, 2022

@jmacd The OTLP exporter is welcome to skip exporting metrics with no data points, I feel. Does this need to be specified?

It would be definitely helpful to explicitly specify this.

Lastly, in the Otel-Go API we avoided the verb "create" so that the expression for a single-shot metric would look more natural,

Well... that's a good point. In the API spec it uses the term "create", I find that most language implementations adopted the term "create" or similar terms as well.

@jmacd
Copy link

jmacd commented Aug 5, 2022

The requested OTLP clarification is in open-telemetry/opentelemetry-specification#2715.

Given that we support duplicate instrument registration, it's not clear that calling "create" will actually create anything. However, I think the use of "create" is still going to be idiomatic in some languages. We decided not to use "create" in the Golang context because Go has a style guideline to avoid superfluous prefixes, e.g., to avoid the "Get" prefix in the accessor "GetXXX()" and prefer just "XXX()".

So, you should be able to have single-shot metrics using the create() method over and over. The requirements for duplicate instrument registration allow this and you'll have no warnings as long as the repeat definitions are the same.

@legendecas legendecas added the signal:metrics Issues and PRs related to general metrics signal label Sep 1, 2022
@github-actions
Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@jpettit
Copy link

jpettit commented Jun 9, 2023

Wanted to provide some additional information on some real world examples of how this issue plays out, in hopes of increasing priority of this issue.

  1. The client SDK in the browser ships the empty data points for delta temporality metrics for every series. In a real world production app this can be many series, and as a result, will cause the queue logic to think it needs to export metrics faster than it normally would (more network requests). This also results in higher resource consumption on client devices due to the additional data throughput. This can be an issue with cellular data plans.

  2. The Collector (at least as of version 75) drops the empty data points from export. However, there are a couple of side effects here. With reduced logging in production we still see log spam for these errors on export. Additionally, the collector still queues up these metrics only to fail exporting them, which can cause batch to fill up faster than it otherwise would. Lastly, this may be my own confusion around the OTC app/host metrics, but it's incredibly difficult to identify "real" problems when logs are overly verbose and batch timeout metrics don't determine the root cause.

@DGuhr
Copy link

DGuhr commented Mar 10, 2024

Jfyi, the python SDK also has this ability. It's unofficial, but it works. Basically setting reader to infinity to stop collecting periodically and calling collect yourself. See this PR for discussion: open-telemetry/opentelemetry-python#3059 I had the use case in a CLI application, and tried it out here currently (hacky thing just to see if it works (it does): https://github.com/DGuhr/cli/tree/otel_integration_hack

@NickBolles
Copy link

NickBolles commented May 7, 2024

  1. The client SDK in the browser ships the empty data points for delta temporality metrics for every series

Agreed, we saw this when creating a POC for OTEL. We have plans for at least 20 metrics, probably many more, and >8 million sessions a day (averaging 15 minutes). That's a lot of wasted requests with empty data points.

I wrote a custom exporter for DELTA accumulation that should:

  • skip the metric if there are no datapoints for the metric,
  • skip the meter if there are no metrics with datapoints
  • skip the request if there are no meters with metrics that have datapoints

Currently this is barely tested - treat this as a POC. I'd love to hear feedback on it though, maybe it can make its way into the OTEL codebase too.

  • Update: I realized the original version didn't handle different datapoint types somehow - the code gets a bit more gross adding those in, but I think it's working more as we expect it
const numericDataPointHasValue = (datapoint: DataPoint<number>) => {
  return datapoint.value !== 0; // todo: this will ignore 0 values, should we?
};
const histogramHasDataPoints = (histogram: DataPoint<Histogram>) => {
  return (
    // histogram.value.buckets.counts.some((count) => count !== 0) ||
    histogram.value.count !== 0
  );
};
const exponentialHistogramHasDataPoints = (histogram: DataPoint<ExponentialHistogram>) => {
  return (
    // histogram.value.negative.bucketCounts.some((count) => count !== 0) ||
    // histogram.value.positive.bucketCounts.some((count) => count !== 0) ||
    histogram.value.count !== 0 || histogram.value.zeroCount !== 0
  );
};

const filterDatapoints = (instrument: MetricData) => {
  const { dataPointType } = instrument;
  switch (dataPointType) {
    case DataPointType.EXPONENTIAL_HISTOGRAM:
      return instrument.dataPoints.filter((datapoint) => exponentialHistogramHasDataPoints(datapoint));
    case DataPointType.HISTOGRAM:
      return instrument.dataPoints.filter((datapoint) => histogramHasDataPoints(datapoint));
    case DataPointType.GAUGE:
    case DataPointType.SUM:
      return instrument.dataPoints.filter((datapoint) => numericDataPointHasValue(datapoint));
    default:
      assertUnreachable(dataPointType);
  }
};

const getMappedInstrument = (instrument: MetricData): MetricData => {
  return {
    ...instrument,
    dataPoints: filterDatapoints(instrument)
  } as MetricData;
};

const getMappedMeter = (meter: ScopeMetrics): ScopeMetrics => {
  const metrics = meter.metrics.map(getMappedInstrument).filter((instrument) => instrument.dataPoints.length > 0);

  return {
    ...meter,
    // filter out metrics with no data points
    metrics
  };
};

const filterMetricsWithNoDatapoints = (metrics: ResourceMetrics): ResourceMetrics => {
  const scopeMetrics = metrics.scopeMetrics
    .map(getMappedMeter)
    // filter out meters with no metrics
    .filter((meter) => meter.metrics.length > 0);
  return {
    ...metrics,
    scopeMetrics
  };
};

class OTLPMetricExporterOnlyWhenNeeded extends OTLPMetricExporter {
  export(metrics: ResourceMetrics, resultCallback: (result: ExportResult) => void) {
    const filteredMetrics = filterMetricsWithNoDatapoints(metrics);
    if (filteredMetrics.scopeMetrics.length > 0) {
      super.export(filteredMetrics, resultCallback);
    } else {
      resultCallback({ code: ExportResultCode.SUCCESS });
    }
  }
}
      new PeriodicExportingMetricReader({
        exporter: new OTLPMetricExporterOnlyWhenNeeded({
          url: `${exporterURL}/v1/metrics`,
          temporalityPreference: AggregationTemporalityPreference.DELTA
        }),
      })

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
never-stale signal:metrics Issues and PRs related to general metrics signal
Projects
None yet
Development

No branches or pull requests

7 participants