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.

Sign up for GitHub

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

Jump to bottom

fix(sdk-metrics): InMemoryMetricExporter clear metrics after shutdown #5214

Open
wants to merge 3 commits into
base: next
Choose a base branch
from
Open

fix(sdk-metrics): InMemoryMetricExporter clear metrics after shutdown #5214

wants to merge 3 commits into from

Conversation

paper2
Copy link
Contributor

@paper2 paper2 commented Nov 27, 2024 •
edited
Loading

Which problem is this PR solving?

Fixes #5131

Short description of the changes

  • make InMemoryMetricExporter reset storing metrics after shutdown

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

The InMemoryExporter does not directly affect the functionality of this library, but I consider it a minor specification change for the library. In fact, I encountered this issue while using InMemoryExporter in another project. If there are tests that retrieve metrics after shutdown, they will fail.

How Has This Been Tested?

  • should get no metrics after shutdown

Checklist:

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

@paper2 paper2 requested a review from a team as a code owner November 27, 2024 22:08
@codecov Codecov
Copy link

codecov bot commented Nov 27, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.63%. Comparing base (47212bb) to head (fd7a69b).
Report is 23 commits behind head on next.

Additional details and impacted files 
@@            Coverage Diff             @@
##             next    #5214      +/-   ##
==========================================
+ Coverage   94.56%   94.63%   +0.06%     
==========================================
  Files         316      323       +7     
  Lines        8037     8085      +48     
  Branches     1628     1643      +15     
==========================================
+ Hits         7600     7651      +51     
+ Misses        437      434       -3
Files with missing lines Coverage Δ
...s/sdk-metrics/src/export/InMemoryMetricExporter.ts 100.00% <100.00%> (+8.69%) ⬆️

... and 38 files with indirect coverage changes

@paper2 paper2 marked this pull request as draft November 27, 2024 22:15
@paper2 paper2 force-pushed the develop/clear-in-memoery-metrics-exporter-data-post-shutdown branch 3 times, most recently from 875ec47 to 5ba87cd Compare November 27, 2024 22:24
@paper2 paper2 changed the title Fixes #5131 Clear InMemoryMetricExporter Data Post-Shutdown to Align with Other Exporters fix(InMemoryMetricExporter) clear metrics after shutdown Nov 27, 2024
@paper2 paper2 force-pushed the develop/clear-in-memoery-metrics-exporter-data-post-shutdown branch from 5ba87cd to 2bf3748 Compare November 27, 2024 22:28
@paper2 paper2 marked this pull request as ready for review November 27, 2024 22:41
@david-luna david-luna self-requested a review November 29, 2024 09:33
david-luna
david-luna reviewed Nov 29, 2024
View reviewed changes
CHANGELOG.md Outdated
@@ -12,6 +12,7 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se
### :boom: Breaking Change

* feat(sdk-metrics): Add support for aggregation cardinality limit with a default limit of 2000. This limit can be customized via views [#5182](https://github.com/open-telemetry/opentelemetry-js/pull/5128)
* fix(InMemoryMetricExporter) clear metrics after shutdown to align with other exporters [#5131](https://github.com/open-telemetry/opentelemetry-js/issues/5131) @paper2
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this should be considered breaking. The API remains intact and it's a fix to have the proper behaviour 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review!! I try to move it 🚀 (Enhancement).

Copy link
Member

Choose a reason for hiding this comment

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

Yes. This is a breaking change for sure. It should go into the 'next' branch, not 'main'

Copy link
Contributor Author

@paper2 paper2 Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This change on CHANGELOG.md has been reverted to breaking change and I have changed target to next branch.

This PR has much conflicts. Should I make a new PR from next branch and add same change it?

@david-luna david-luna changed the title fix(InMemoryMetricExporter) clear metrics after shutdown fix(sdk-metrics): InMemoryMetricExporter clear metrics after shutdown Nov 29, 2024
@david-luna david-luna self-requested a review November 29, 2024 09:36
@david-luna
Copy link
Contributor

@paper2 I've updated the title to follow the guidelines :)

@paper2 paper2 force-pushed the develop/clear-in-memoery-metrics-exporter-data-post-shutdown branch 2 times, most recently from bbd9b5f to b3c14d5 Compare November 29, 2024 22:19
pichlermarc
pichlermarc previously requested changes Dec 1, 2024
View reviewed changes
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Please re-target the PR to 'next' - it may be breaking for end-users that rely on this behavior in integration tests.

@paper2 paper2 changed the base branch from main to next December 2, 2024 23:17
@paper2 paper2 force-pushed the develop/clear-in-memoery-metrics-exporter-data-post-shutdown branch from 6db79c4 to be236d7 Compare December 2, 2024 23:20
@paper2 paper2 changed the base branch from next to main December 2, 2024 23:20
@paper2 paper2 force-pushed the develop/clear-in-memoery-metrics-exporter-data-post-shutdown branch from be236d7 to c91f1d7 Compare December 2, 2024 23:21
@paper2 paper2 changed the base branch from main to next December 2, 2024 23:28
@david-luna
Copy link
Contributor

@paper2

There are some conflicts in your PR and #5236 will be merged soon. I suggest you wait for it before dealing with the conflicts.

@paper2
Merge branch 'next' of github.com:open-telemetry/opentelemetry-js int…
70d0935 
…o develop/clear-in-memoery-metrics-exporter-data-post-shutdown
@paper2 paper2 requested a review from pichlermarc December 7, 2024 23:25
@pichlermarc pichlermarc dismissed their stale review December 9, 2024 09:05

I need to take another look

pichlermarc
pichlermarc reviewed Dec 11, 2024
View reviewed changes
packages/sdk-metrics/test/export/InMemoryMetricExporter.test.ts Outdated
await exporter.shutdown();

const otherMetrics = exporter.getMetrics();
assert.ok(otherMetrics.length === 0);
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
assert.ok(otherMetrics.length === 0);
assert.strictEqual(otherMetrics.length, 0);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I have fixed it.
use strictEqual function instead of ok

Other assert.ok(xxx === yyy) expressions exists in this test file. Should I fix them too??

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pichlermarc pichlermarc pichlermarc left review comments

@david-luna david-luna david-luna approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Proposal: Clear InMemoryMetricExporter Data Post-Shutdown to Align with Other Exporters
3 participants
@paper2 @david-luna @pichlermarc