PeriodicExportingMetricsReader with value = infinity to support explicit metric collection

[howardyoo]

Description

In this PR, I would like to introduce a new feature in PeriodicExportingMetricsReader. Basically, it will have an additional feature which will allow the reader to receive zero interval (explicitly set by user). When the interval is set to zero, the PeriodicExportingMetricReader will not be automatically triggered to collect metrics in intervals, but rather rely on user to explicitly invoke collect to do so. The purpose is to support cases where user would rely on external triggering mechanism to perform collection.

Fixes # (issue)
#3055

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

• openteleemtry-sdk/tests/metrics/test_periodic_exporting_metric_reader.py

Does This PR Require a Contrib Repo Change?

Answer the following question based on these examples of changes that would require a Contrib Repo Change:

• The OTel specification has changed which prompted this PR to update the method interfaces of opentelemetry-api/ or opentelemetry-sdk/

• The method interfaces of test/util have changed

• Scripts in scripts/ that were copied over to the Contrib repo have changed

• Configuration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in

  • pyproject.toml
  • isort.cfg
  • .flake8

• When a new .github/CODEOWNER is added

• Major changes to project information, such as in:

  • README.md
  • CONTRIBUTING.md

• Yes. - Link to PR:

• No.

Checklist:

• Followed the style guidelines of this project
• Changelogs have been updated
• Unit tests have been added
• Documentation has been updated

[howardyoo]

Hey folks, thanks for discussing this PR with me.
I'm just wondering... to see if it would be okay for me to implement a logic on existing PeriodicMetricReader, such that when given interval of 0, it will simply not trigger any collection/export. If that's the case, I can update my commit on this branch accordingly.

[srikanthccv]

I still think the MetricReader should address this i.e it's implementation should be complete and one can collect dynamically. I would like to hear others' thoughts on the spec interpretation.

[howardyoo]

Hi all, thank you for all your interest in this PR.
I have looked and contemplated, and so decided to modify my PR to not introduce a new ExportingMetricsReader, but rather modify the existing reader to support zero interval (suggested by @lzchen ), such that by setting up the interval as zero, it will not run a thread to trigger the collect() in set interval. This does meet the requirements, so I've modified the code with additional commits. I hope this PR would get the review and go through. Thank you!

[lzchen]

@howardyoo

Change looks good. Could you add a working example docs folder under metrics to show a user how to call collect explicitly? Also, please add an entry to the CHANGELOG.

[lzchen]

@howardyoo

Also as a addition to a followup conversation I had with @srikanthccv, we don't want to set a precedence of advising users normally to use collect explicitly with the PeriodicExportingMetricReader. Could you also include a comment explicitly in the example to warn users of the implications of using collect() manually? Something along the lines of: "Warning: Setting a time interval of 0 allows you to export metric records manually. Calling collect() explicitly is not officially recommended by OpenTelemetry in conjunction with the PeriodicExportingMetricReader. Use at your own risk"

[howardyoo]

@howardyoo

Also as a addition to a followup conversation I had with @srikanthccv, we don't want to set a precedence of advising users normally to use collect explicitly with the PeriodicExportingMetricReader. Could you also include a comment explicitly in the example to warn users of the implications of using collect() manually? Something along the lines of: "Warning: Setting a time interval of 0 allows you to export metric records manually. Calling collect() explicitly is not officially recommended by OpenTelemetry in conjunction with the PeriodicExportingMetricReader. Use at your own risk"

Yes, I believe I can do that, so will include this in the example. Thank you!

[lzchen]

@howardyoo

Actually after discussion it in our Python SIG, we've decided that it's better to treat this functionality as an internal implementation, rather than an actual feature, so it would be preferred to not have an explicit example for it. Apologies for the back and forth.

[aabmass]

Thanks for the PR @howardyoo. I have a concern with the magic value being zero. My expectation with a zero interval would be that metrics are constantly exported. I probably wouldn't recommend users doing that, but I can imagine someone opening an issue with this request, e.g. they only have asynchronous instruments and want to scrape as quickly as possible.

In the case of zero interval, you could also avoid creating a background thread as there is no waiting needed between calls.

[howardyoo]

@howardyoo

Actually after discussion it in our Python SIG, we've decided that it's better to treat this functionality as an internal implementation, rather than an actual feature, so it would be preferred to not have an explicit example for it. Apologies for the back and forth.

Ok, I'm good with that. Updated commit by removing the examples and mention of it in the README.

[howardyoo]

Thanks for the PR @howardyoo. I have a concern with the magic value being zero. My expectation with a zero interval would be that metrics are constantly exported. I probably wouldn't recommend users doing that, but I can imagine someone opening an issue with this request, e.g. they only have asynchronous instruments and want to scrape as quickly as possible.

In the case of zero interval, you could also avoid creating a background thread as there is no waiting needed between calls.

I see,
However, in that case, setting the zero value to constantly invoke collect without any intervals is quite dangerous and impractical, and thus needs to be avoided. In many other API's setting zero interval either disables certain interval feature, or even make the interval infinite, so if we could set the comments to let the user know properly, this would actually be a safer feature, in my opinion.

[aabmass]

Thanks for working through this PR! One nit, otherwise LGTM

[howardyoo]

Hey! Just wanted to say thanks to @srikanthccv , @lzchen , @ocelotl , and @aabmass for all the thoughtful comment and reviews, approvals to make this go through. Really appreciated your support and feedbacks!

[srikanthccv]

There was a lot of back and forth from us, and thanks for being patient all along.

[santosh-kodali]

when will the next release be?
I need this feature greatly

[srikanthccv]

probably next week

[DGuhr]

Hey 👋 Stumbled upon this while trying to send a gauge from a short-lived CLI command. That gauge should be sent when you run mycli test <options> --export <endpoint> --format=otlp.

I thought this would be a perfect, even if unofficial, solution, but when I try to set the export_interval_millis to zero, I get the following error: ValueError: interval value 0 is invalid and needs to be larger than zero and lower than infinity.. So, it'd be great if anyone could tell me an alternative way on how to achieve one-time sending for short-lived programs like a CLI. The CLI should run in build pipelines and only send a status on the test/validation once every 24h and then exit.

Edit: looking at the code, the error message is a bit misleading, as math.inf works. nvm :)

[howardyoo]

Hi, I believe we had some means to do what you want to do, and I think we made the changes. Let me check to see whether that has been done.HowardSent from my iPhoneOn Mar 9, 2024, at 9:53 AM, Dominik Guhr ***@***.***> wrote: Hey 👋 Stumbled upon this while trying to add a gauge to a CLI that is sent when you run mycli test --export --format=opentelemetry - I thought this would be a perfect, even if unofficial, solution, but when I try to set the export_interval_millis to zero, I get an error that it is not allowed to be used with zero nor infinity with the current api/sdk. So, it'd be great if anyone could tell me if the manual triggering still works or not. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[howardyoo]

An update:This was done about a year ago, so spent some time going back threads to read the history. So just to explain, setting the interval to zero was suggested originally but met a better suggestion of instead of setting to math.inf. If the math.inf was set as an interval, the periodic metric exporter should not get triggered periodically, and thus would need to be exported manually.Hope this helps!Sent from my iPhoneOn Mar 9, 2024, at 9:53 AM, Dominik Guhr ***@***.***> wrote: Hey 👋 Stumbled upon this while trying to add a gauge to a CLI that is sent when you run mycli test --export --format=opentelemetry - I thought this would be a perfect, even if unofficial, solution, but when I try to set the export_interval_millis to zero, I get an error that it is not allowed to be used with zero nor infinity with the current api/sdk. So, it'd be great if anyone could tell me if the manual triggering still works or not. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[DGuhr]

Thanks for the clarification @howardyoo - yes, I figured it out by looking at the code for the Periodic metrics exporter. It was just that I trusted the attribute errors message too much after 0 didn't work- as written above, it states that math.inf also wouldn't work, so I expected to get the very same error using it.
Turns out I did not. I guess it's like this on purpose, because "unofficial", but I'd argue there are more use cases coming in when things like buildpipeline-observability are growing in adoption. Though, what do I know 😅 Thanks again. All good here 👍

[howardyoo]

Thanks for the clarification @howardyoo - yes, I figured it out by looking at the code for the Periodic metrics exporter. It was just that I trusted the attribute errors message too much after 0 didn't work- as written above, it states that math.inf also wouldn't work, so I expected to get the very same error using it. Turns out I did not. I guess it's like this on purpose, because "unofficial", but I'd argue there are more use cases coming in when things like buildpipeline-observability are growing in adoption. Though, what do I know 😅 Thanks again. All good here 👍

I actually went back and checked, and found out the error message stated: needs to be larger than zero and lower than infinity., which is kind of not true (since math.inf would actually be accepted). However, I'm not sure whether there is a need to have more details on the message itself, like : needs to be larger than zero and can or lower than infinity. After thinking about this a bit more, I guess it might be worth raising an issue to switch this message.