The committers listed above are authorized under a signed CLA.

• ✅ login: Jayclifford345 / name: Jay Clifford (786e861, f37e240, 2985830, 8fed0a5, e9127fc, a022a6b, f20eea6, ac2a4a0, 586f6cf, 3ed6a98, 62132ef, 95ab7e5, d9af8a0, 312e36e, d2c0d4d, 5c02c49, 7926bb4, 7653c9b)

Thanks for submitting!

What other benefits come with this new Reader, instead of using PeriodicExportingMetricReader with export_interval_millis=math.inf

Hi @tammy-baylis-swi! Thanks for looking into this. You raise a fair point on using export_interval_millis=math.inf. My design choice was to emulate a similar structure to the batch exporter for logs, where you have direct control over batch size and queue. This works well when you don't have an Otel collector available but still plan to use an Otel-compatible endpoint. I assume the periodic workaround method would be:

1. Set export interval to inf (which skips thread creation)
2. Invoke collection which will immediately export

I would be happy to look into merging the queue system into periodic reader if that makes more sense. If not I would be happy to contribute an example of this to the repo to make the feature more explicit.

I assume the periodic workaround method would be:

1. Set export interval to inf (which skips thread creation)
2. Invoke collection which will immediately export

Yes correct.

you have direct control over batch size and queue

That's great actually!!

merging the queue system into periodic reader if that makes more sense

Imo it'd be nice to keep this separate as an additional concrete Reader class like you have it, like with exporters/processors for logs and traces.

Please could you have a check of the failing tests. There's a few failures like

        # Verify success and metrics exported
        self.assertTrue(success)
>       self.assertEqual(len(exporter.metrics), 1)
E       AssertionError: 4 != 1

Hi @tammy-baylis-swi, yes I shall look into the test. I think I had some weird logic which in the test which was passing locally but not here. Will get that fixed Monday. Documentation wise is there anything I need to do? I noticed that the documentation was being built but I couldn't find a specific page for the SDK. I assume part of it is being built from the inline class description?

Hi @tammy-baylis-swi,
That should be the fixes to the tests and some of the batching logic. I realised there was a fundamental issue with how I was "batching" (should have stuck by the not shipping code on fridays) metric samples which should now be resolved. Let me know what you think :)

Hi @Jayclifford345 , yes the docstrings are used to generate some of the docs with sphinx -- style guide links in the contributing.md. For the new example (much appreciated), the new class is part of the SDK so I don't think doc-requirements.txt nor docs/examples/metrics/reader/requirements.txt need to be updated.

I think you need to do a local tox -e docs and push the updates if you haven't already.

Thanks for the test updates! Those util-http I don't think are to do with your changes.

Hi @tammy-baylis-swi, can confirm the new reader is showing up in the docs as expected after building.

The failed test seems to be a mismatch in the workflow, not being able to find a specific commit hash. Not quite sure how to resolve that one.

Shall we try to run the test again and see if it clears up on its own?

@Jayclifford345 Please merge main into your branch, then I'll re-run

Also: Please could you create an accompanying Issue (Feature Request) to link to this PR! https://github.com/open-telemetry/opentelemetry-python/issues

Hi @tammy-baylis-swi,
That is main merged and here is the issue: #4549. Ready for the next run. Thanks again for being persistent with this!

Hmm @tammy-baylis-swi, I checked the error

Check that anchor links are lowercase
I did an initial check and couldn't see any uppercase

- Add `setuptools` to `install_requires`
  ([#2334](https://github.com/open-telemetry/opentelemetry-python/pull/2334))
- Add otlp entrypoint for log exporter
  ([#2322](https://github.com/open-telemetry/opentelemetry-python/pull/2322))
- Support insecure configuration for OTLP gRPC exporter
  ([#2350](https://github.com/open-telemetry/opentelemetry-python/pull/2350))

Thanks a lot @tammy-baylis-swi! I added a note to the agenda look forward to meeting you there :)

I checked the otel specfication and sadly it looks like this would be out of current scope since we already implemented all of the required metrics readers: https://opentelemetry.io/docs/specs/otel/metrics/sdk/#metricreader

Happy to pull the plug if this is more trouble than it`s worth. I can just open a new PR to give an example of the alternative method if that helps instead? Let me know what you think though after review

@Jayclifford345

Apologies for the late review. It seems to me the only difference between this functionality and the current workaround in the periodic metric reader (with infinity) is the batching capability. While I do agree that our current components does not satisfy your specific situation, I have reservations in adding new components to the already stable api/sdk package, especially ones that are not in the spec. We will also have to support this metric reader forever (even past your use case) in case other people take a dependency on it. I'd be much more likely to approve this if we perhaps make this an experimental component (with an underscore) with guidance and documentation indicating to users to try at their own risk. wdyt?

Hi @lzchen, I absolutely agree with your comments above. Sorry, I couldn't attend the Python sig meeting yesterday. I am happy to turn it into an experimental feature and see if it lands for the otel community. I want to make sure that I am not putting you all in a bad spot though where we are moving away from the specification outlined. I am happy to support this feature in the meantime. I ended up creating an example with the workaround here as an official method: #4559. I will work on the changes to get it moved into an experimental mode. Let me know though if there are any processes I need to follow in the meantime with regards to the specification :). Thanks again for the review.

I think, based on the discussion in the issue, this is out of scope for now and with the given workaround now documented in #4559 I think this works as a good alternative. If this arises again in the future I am happy for this PR to be reopened and the code used in another implementation.