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

testing: add in-memory metrics exporter #653

Merged
merged 8 commits into from
May 15, 2020

Conversation

codeboten
Copy link
Contributor

Adding InMemoryMetricsExporter to help with unit-tests, as part of the change i'm also adding meter_provider and memory_metrics_exporter to TestBase

@codeboten codeboten requested review from mauriciovasquezbernal and a team May 5, 2020 23:45
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

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

Thanks a lot for these changes, those are quite useful.

I think we should split up the TestBase currently the class has some specific logic for tracing, check_span_instrumentation_info and I think in the future there will more, like checking parent-child relationship for instance.

I think there should be a common TesBase class that inherits from unittest.TestCase, then we could have TestBaseTracer, TestBaseMetrics. What do you think?

with self._lock:
self._exported_metrics.extend(metric_records)
return MetricsExportResult.SUCCESS

Choose a reason for hiding this comment

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

What do you think about implementing the shutdown method as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it, but it wasn't clear to me what shutdown would be used for yet.

Choose a reason for hiding this comment

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

I think it's just a matter of completeness, it could help to find bugs if an implementation is calling shutdown by mistake, because it'll reject further calls to export.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

tests/util/src/opentelemetry/test/test_base.py Outdated Show resolved Hide resolved
Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>
@codeboten
Copy link
Contributor Author

I think there should be a common TesBase class that inherits from unittest.TestCase, then we could have TestBaseTracer, TestBaseMetrics. What do you think?

I can imagine how this will be useful in the future, but I'm also not sure how much TestBaseTracer and TestBaseMetrics will have in common.

@mauriciovasquezbernal
Copy link
Member

Probably not much, I can imagine very generic helpers like:

@staticmethod
@contextmanager
def disable_logging(highest_level=logging.CRITICAL):
logging.disable(highest_level)
try:
yield
finally:
logging.disable(logging.NOTSET)


def __init__(self):
self._exported_metrics = []
self._stopped = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like self._stopped isn't being used for anything? I believe in the span exporter it is used as a check upon calling export.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added the check to export, as is done in the InMemorySpanExporter

@toumorokoshi
Copy link
Member

Thanks a lot for these changes, those are quite useful.

I think we should split up the TestBase currently the class has some specific logic for tracing, check_span_instrumentation_info and I think in the future there will more, like checking parent-child relationship for instance.

I think there should be a common TesBase class that inherits from unittest.TestCase, then we could have TestBaseTracer, TestBaseMetrics. What do you think?

This would be cool, and I think this is precisely why we need to add pytest fixtures. It is not possible easily compose fixture setup / teardown using classes, so the join of the two will require careful coding to perform correctly (calling all base classes' setup and teardown methods).

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

I think a quick decision on export() and Leighton's note about the unused _stopped variable and I'm happy to approve. Thanks!

mauriciovasquezbernal and others added 5 commits May 7, 2020 21:21
- Convert file to rst to be included in the generated documentation
- Include docs for main module.
- Use internal reference instead of read the docs link
Initial Instrumentation

Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>
Co-authored-by: Mathieu Hinderyckx <mathieu.hinderyckx@gmail.com>
Co-authored-by: alrex <alrex.boten@gmail.com>
Co-authored-by: Yusuke Tsutsumi <yusuke@tsutsumi.io>
Adding initial aiohttp client.

This module is only supported on Python3.5, which is the oldest supported by
aiohttp.

Co-authored-by: Yusuke Tsutsumi <yusuke@tsutsumi.io>
Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Thanks!

@codeboten codeboten merged commit 5913d4a into open-telemetry:master May 15, 2020
owais pushed a commit to owais/opentelemetry-python that referenced this pull request May 22, 2020
Adding InMemoryMetricsExporter to help with unit-tests, as part of the change i'm also adding meter_provider and memory_metrics_exporter to TestBase

Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
…-telemetry#653)

* fix: do not load plugin when they patch a different module than defined in config open-telemetry#626
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.

6 participants