Basic protection against the unintended cardinality explosion #3486
Conversation
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py
Outdated
Show resolved
Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py
Outdated
Show resolved
Hide resolved
| @@ -96,6 +96,7 @@ def __init__( | |||
| attribute_keys: Optional[Set[str]] = None, | |||
| aggregation: Optional[Aggregation] = None, | |||
| instrument_unit: Optional[str] = None, | |||
| aggregation_cardinality_limit: Optional[int] = None, | |||
There was a problem hiding this comment.
Looks like these args are individually documented in the docstring above. Should we add one for this arg?
| class TestMetricCardinalityLimit(TestCase): | ||
|
|
||
| def setUp(self): | ||
| self.exporter = InMemoryMetricExporter() |
There was a problem hiding this comment.
Not sure, but if there were multiple tests in this class at some point, setUp might be vestigial now.
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/_view_instrument_match.py
Outdated
Show resolved
Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/__init__.py
Outdated
Show resolved
Hide resolved
| self.meter_provider = MeterProvider(metric_readers=[self.reader]) | ||
| self.meter = self.meter_provider.get_meter("test_meter") | ||
|
|
||
| def test_metric_cardinality_limit(self): |
There was a problem hiding this comment.
If we are introducing the ability to configure the value we should have a test for that too.
|
|
||
| class _ViewInstrumentMatch: | ||
| def __init__( | ||
| self, | ||
| view: View, | ||
| instrument: Instrument, | ||
| instrument_class_aggregation: Dict[type, Aggregation], | ||
| aggregation_cardinality_limit: Optional[int], |
There was a problem hiding this comment.
I'm curious as to why _ViewInstrumentMatch, MetricReaderStorage and View all have their separate configurations for aggregation_cardinality_limit. Shouldn't this be only configured from View and propagated down?
|
|
||
| def setUp(self): | ||
| self.exporter = InMemoryMetricExporter() | ||
| self.reader = PeriodicExportingMetricReader(exporter=self.exporter, export_interval_millis=500) |
There was a problem hiding this comment.
From Srikanth's comment on the other PR, can we just use InMemoryMetricReader instead? Here is an example from another test https://github.com/open-telemetry/opentelemetry-python/blob/91ac1bea6ed2b50e7e17b91cec2770674f869517/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_export.py#L25C9-L39
…ic_reader_storage.py Co-authored-by: Pablo Collins <pablo.collins@gmail.com>
Description
This pull request adds the limit on the metrics to prevent the unintended cardinality explosion. For more information, see #2700
New tests are added for this functionality. Additionally, I moved the
InMemoryMetricExporterso it could be used for more tests, and added tests for it.Fixes #2700
Type of change
Please delete options that are not relevant.
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
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/oropentelemetry-sdk/The method interfaces of
test/utilhave changedScripts in
scripts/that were copied over to the Contrib repo have changedConfiguration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in
pyproject.tomlisort.cfg.flake8When a new
.github/CODEOWNERis addedMajor changes to project information, such as in:
README.mdCONTRIBUTING.mdNo.
Checklist: