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

Specify MetricReader defaults for stdout MetricExporter #2415

Conversation

alanwest
Copy link
Member

@alanwest alanwest commented Mar 14, 2022

This is a follow up from #2379 (comment).

The .NET SIG has observed that users generally expect an interval shorter than 60 seconds for metrics to be emitted. This PR proposes a default interval of 10s for the stdout exporter. These values are simply a starting place for discussion.

@reyang
Copy link
Member

reyang commented Mar 15, 2022

@jmacd asked the question during the 3/15/2022 spec SIG meeting "if in-memory-exporter has export period equals to Infinity, does this make it essentially a Pull Exporter?".

Here is my thinking - my short answer is NO, long answer:

In the spec we have the wording "Pull Metric Exporter reacts to the metrics scrapers and reports the data passively" and "Push Metric Exporter sends metric data it receives from a paired MetricReader".

I think the key differentiator is that a Pull Exporter can only export metrics under certain condition, and if that condition is not met, the export action would make no sense. For example, if there is no scraping, it makes no sense to export via a Prometheus Exporter (it doesn't make sense for a Prometheus Exporter to store the data somewhere and wait for the next scraping event). On the other hand, a Push Exporter can respond to export action at any time, even if the actual delivery mechanism is unavailable (e.g. OTLP Exporter with "connection refused" could either return SUCCESS by storing the data on a local persistent storage and promising to send the data when the network is back, or return FAILED so the caller knows).

@aabmass
Copy link
Member

aabmass commented Mar 15, 2022

@reyang when using an in memory exporter for testing (e.g. assert my application generated metrics), the "scraper" would be the actual test case making an assertion. I still think pull exporter is the best way to model this.

The only time I can think of it being useful as a push exporter is when testing the periodic exporting metric reader.

@reyang
Copy link
Member

reyang commented Mar 16, 2022

@reyang when using an in memory exporter for testing (e.g. assert my application generated metrics), the "scraper" would be the actual test case making an assertion. I still think pull exporter is the best way to model this.

The only time I can think of it being useful as a push exporter is when testing the periodic exporting metric reader.

@aabmass I think we're talking about the exact same thing 😄 Maybe just the English wording - a push exporter is something that we can always trigger whenever we want (essentially what you referred to as "pull").

@reyang reyang added spec:metrics Related to the specification/metrics directory area:sdk Related to the SDK labels Mar 16, 2022
@reyang reyang added this to the Metrics API/SDK Stable Release milestone Mar 16, 2022
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

@reyang I'm not sure I understand still, the only use case for in-memory exporter i see in the spec is unit testing. A pull exporter would immediately return metrics to the "scraper" (in this case the unit test). The push exporter would require you to call MetricReader.collect() in the unit test, then it would save the metrics in memory. Then you can access the buffered metrics.

For unit testing purposes, the former seems easier. Can an SDK implement the in memory exporter as a pull exporter?

specification/metrics/sdk_exporters/in-memory.md Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor

jmacd commented Mar 16, 2022

@aabmass This doesn't directly answer your question, but in #2404 I've tried to distinguish the explicit process of creating an exporter and somehow giving it a reader, which is done through an API that the user calls, vs the process of automatically registering an exporter which is done by the SDK according to environment variables. So it's either the user (explicit) or the SDK (automatic) that inputs the pairing information.

There is a persistent question about factories associated with MetricReader instances, because the pairing happens after the SDK is created. We have not specified it because it is an implementation detail, but there is an implicit method that every SDK has independently discovered which is the process for creating MetricReaders, and it goes like this: (1) something creates an exporter, (2) the reader's class and parameters are configured (i.e., the "factory") (3) the SDK is created, (4) the Reader instance is created and a binding between the exporter and the SDK that is aware of the reader parameters is constructed. This "binding" method is sometimes called Apply() or Register(), it's called by and for the SDK to hand the reader the "interface value" it will use to trigger collection.

Summarizing, it's either the user or the SDK that determines MetricReader parameters, and we keep talking about Reader factories because while the Exporter, the class of Reader, and the Reader parameters may be configured by the user (or auto-configured by the SDK), it's the SDK that registers (and typically creates) Reader instances.

@jmacd
Copy link
Contributor

jmacd commented Mar 16, 2022

@reyang @alanwest Re: in-memory exporter

It sounds like we're trying to make a fine distinction here, not sure it matters in practice.

A "fully assembled" reader and exporter, whether pull-based or push-based, let's say is going to perform one collection and perform one pass through the data. The form is ExposeData(CollectMetrics()). The distinguishing feature of a push-based exporter is that CollectMetrics() is decoupled from ExposeData(), whereas for pull-based exporters the two are called synchronously.

When using an in-memory exporter, it seems like both push-based and pull-based semantics could be useful. I think it's more likely that tests want to CollectMetrics() at the same moment as it inspects the exposed data (as with pull-based), less likely that tests want to inspect the exposed data from the last explicit or periodic collection (as with push-based).

@jack-berg
Copy link
Member

the only use case for in-memory exporter i see in the spec is unit testing. A pull exporter would immediately return metrics to the "scraper" (in this case the unit test). The push exporter would require you to call MetricReader.collect() in the unit test, then it would save the metrics in memory. Then you can access the buffered metrics.

FWIW, in the java SDK we currently have both a pull and a push based versions:

  • InMemoryMetricReader is pull based and is used all over the place in tests.
  • InMemoryMetricExporter can be paired with a periodic metric reader and acts as a push based exporter that tests can retrieve results from. Its used in only 1 or 2 places. I would argue we could get rid of this one.

@alanwest
Copy link
Member Author

Just taking a step back here and asking a question that maybe has already been settled long ago...

Is it important that we have a spec for an in-memory exporter?

If it is, then I'd be inclined to open a separate issue to further the discussion of whether the in-memory exporter should be a push exporter, pull exporter, or that SDKs should have both flavors. As it stands the spec states that it is a push exporter.

I think the behavior of the console exporter is the most important part of this PR as it is often the first exporter a user picks up to see what the SDK does for them. Is anyone aware of use cases where end users are reaching for the in-memory exporter?

@alanwest alanwest changed the title Specify MetricReader defaults for stdout/in-memory MetricExporter Specify MetricReader defaults for stdout MetricExporter Mar 18, 2022
@alanwest
Copy link
Member Author

I've updated the PR to leave the in-memory exporter spec as-is for now. I'll open an issue shortly so we can follow up on the discussion about it separately.

@reyang
Copy link
Member

reyang commented Mar 18, 2022

@reyang I'm not sure I understand still, the only use case for in-memory exporter i see in the spec is unit testing. A pull exporter would immediately return metrics to the "scraper" (in this case the unit test). The push exporter would require you to call MetricReader.collect() in the unit test, then it would save the metrics in memory. Then you can access the buffered metrics.

For unit testing purposes, the former seems easier. Can an SDK implement the in memory exporter as a pull exporter?

It seems to me that we are actually talking about the same thing but trapped by English. Let me try to explain:

In the SDK spec we say Unlike Push Metric Exporter which can send data on its own schedule, pull exporter can only send the data when it is being asked by the scraper, and ForceFlush would not make sense. I think the key difference between push and pull is that a push exporter can be flushed/pulled at anytime. In memory exporter can be flushed during the unit test based on the actual test scenario.

Here goes some C# example code to demonstrate this: https://github.com/open-telemetry/opentelemetry-dotnet/blob/bdcf942825915666dfe87618282d72f061f7567e/test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs#L47-L55 - the in memory exporter doesn't have any interval output behavior here, if the test case want to export something, it can explicitly call meterProvider.ForceFlush (which will flush all exporters; and if the intention is to just flush one specific exporter, folks can call exporter.ForceFlush), and we call it "push" rather than "pull".

Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Thanks for opening an issue for the in memory exporter @alanwest. This PR LGTM

@reyang
Copy link
Member

reyang commented Mar 21, 2022

@alanwest CI failed due to a dead link, would you resolve it? Thanks!

@reyang
Copy link
Member

reyang commented Mar 21, 2022

The CI error doesn't seem to be introduced by this PR:

  ERROR: 1 dead links found!
  14 links checked.
  [✖] #attribute-limits-configuration → Status: 404
make: *** [Makefile:31: markdown-link-check] Error 1
Error: Process completed with exit code 2.

@reyang reyang mentioned this pull request Mar 21, 2022
@reyang reyang merged commit 93a79f8 into open-telemetry:main Mar 22, 2022
@alanwest alanwest deleted the alanwest/stdout-inmemory-metric-reader-defaults branch March 23, 2022 20:34
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants