[PoC] alternate approach for InMemoryExporter, adding selective copy to Metric (for discussion) #3187
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I got latest on main and now some OTPL tests broke. This highlights the flaw with the selective approach. |
CodeBlanch
reviewed
Apr 15, 2022
| { | ||
| this.MetricType = other.MetricType; | ||
| this.InstrumentIdentity = other.InstrumentIdentity; | ||
| this.aggStore = other.aggStore.Copy(); |
There was a problem hiding this comment.
One concern with this approach. AggregatorStore holds onto a pretty sizeable buffer (sizeof(MetricPoint) * 2000 essentially) so user might OOM if their inmemoryexporter is running long enough?
There was a problem hiding this comment.
good point!
It also looks like there's a circular reference with AggregatorStore appearing on both Metric and MetricPoint.
Codecov Report
@@ Coverage Diff @@
## main #3187 +/- ##
==========================================
- Coverage 85.17% 84.95% -0.22%
==========================================
Files 259 259
Lines 9330 9347 +17
==========================================
- Hits 7947 7941 -6
- Misses 1383 1406 +23
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alternate approach for #2361
Changes
MetricandAggregatorStore.InMemoryExporterto copyMetric.Discussion
This attempts to meet the initial ask of #2361 by providing a selective deep-copy of
Metric."Selective" meaning the minimum fields/properties that are required for current unit tests.
A SME might recommend additional minimum members.
With this draft, all unit tests pass locally.
Ideally, we should deep-copy ALL members.
I would not recommend this approach without also investing in thorough unit tests to fully validate copying.
I don't want to give the false impression this this is a true deep-copy when members could be missed!
This will likely require some extensive reflection-based comparisons and I want to test the waters before investing in this.
I've already evaluated one library (FluentAssertions) to do this comparison, but this has some limitations.
FluentAssertions won't review private members and it depends on the Equals() method for structs.
I'm open to suggestions for other libraries!
Open Questions
publicto better communicate what is/isn't copied?For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes