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

Refactor InMemoryMetricExporter and add tests #3519

Conversation

nstawski
Copy link
Contributor

@nstawski nstawski commented Nov 8, 2023

Description

This is a pull to separate the refactor of InMemoryMetricExporter from its usage in the pull#3486

Type of change

  • New feature (non-breaking change which adds functionality)

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

  • Added tests for InMemoryMetricExporter

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added

@nstawski nstawski requested a review from a team November 8, 2023 18:38
@nstawski
Copy link
Contributor Author

nstawski commented Nov 8, 2023

Don't think this pull needs the changelog update, but happy to add it if necessary.

@nstawski nstawski changed the title Refactor and add tests for the InMemoryMetricExporter Refactor InMemoryMetricExporter and add tests Nov 9, 2023
Copy link
Member

@pmcollins pmcollins left a comment

Choose a reason for hiding this comment

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

LGTM with one suggestion.


This class can be used for testing purposes. It stores the exported metrics
in a dictionary in memory, indexed by an auto-incrementing counter that can
be retrieved using the `metrics` attribute.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
be retrieved using the `metrics` attribute.
be retrieved using the `_counter` attribute.

Copy link
Member

Choose a reason for hiding this comment

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

If you did intend to highlight the metrics attribute, you could rephrase the text to something like:

This class is designed for testing purposes. It maintains exported metrics in the metrics dictionary attribute.

@srikanthccv
Copy link
Member

I don't think we need to add InMemoryMetricExporter to the public API. The InMemoryMetricReader is what should be used for testing. What problem does InMemoryMetricExporter solve that InMemoryMetricReader can't do?

@ocelotl
Copy link
Contributor

ocelotl commented Feb 9, 2024

I don't think we need to add InMemoryMetricExporter to the public API. The InMemoryMetricReader is what should be used for testing. What problem does InMemoryMetricExporter solve that InMemoryMetricReader can't do?

I agree, @nstawski, I'm closing this PR because of the reasons above, please let me know if you disagree 👍

@ocelotl ocelotl closed this Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants