Skip to content

Fix metrics context circular reference#1535

Merged
esigo merged 16 commits intoopen-telemetry:mainfrom
esigo:metrics-context-circular-reference
Aug 11, 2022
Merged

Fix metrics context circular reference#1535
esigo merged 16 commits intoopen-telemetry:mainfrom
esigo:metrics-context-circular-reference

Conversation

@esigo
Copy link
Copy Markdown
Member

@esigo esigo commented Jul 31, 2022

Fixes #1534 (issue)

Changes

removed the cycles by using a wrak_ptr to the context in meter. Also meter_collector can't have weak_ptr to the context so I used raw pointer.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 31, 2022

Codecov Report

Merging #1535 (a4052af) into main (86c9f2b) will decrease coverage by 0.57%.
The diff coverage is 43.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1535      +/-   ##
==========================================
- Coverage   83.85%   83.28%   -0.56%     
==========================================
  Files         156      156              
  Lines        4908     4970      +62     
==========================================
+ Hits         4115     4139      +24     
- Misses        793      831      +38     
Impacted Files Coverage Δ
sdk/src/metrics/meter.cc 4.80% <0.00%> (-0.46%) ⬇️
sdk/src/metrics/state/observable_registry.cc 14.29% <0.00%> (-0.86%) ⬇️
sdk/src/metrics/sync_instruments.cc 79.24% <55.77%> (-16.00%) ⬇️
sdk/src/metrics/state/metric_collector.cc 48.00% <66.67%> (-2.00%) ⬇️
sdk/src/metrics/meter_context.cc 42.86% <100.00%> (ø)

@lalitb
Copy link
Copy Markdown
Member

lalitb commented Aug 8, 2022

@esigo - I have gone through the circular reference between meter_context <-> meter, and meter_context <-> metric_collector, and I don't think it would be easy to avoid them. These changes look good in that scenario. We can have a separate issue to track the redesign requirement for metrics v1.0 / GA.

@esigo esigo changed the title [WIP] Fix metrics context circular reference Fix metrics context circular reference Aug 9, 2022
@esigo esigo marked this pull request as ready for review August 9, 2022 17:29
@esigo esigo requested a review from a team August 9, 2022 17:29
void *state = nullptr)
{
auto view_registry = meter_context_->GetViewRegistry();
auto ctx = meter_context_.lock();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we be checking if the ctx is valid here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

makes sense. Shall we log something too?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes, I think we should log here if meter context is no more valid.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

Comment thread sdk/src/metrics/meter.cc Outdated
InstrumentDescriptor &instrument_descriptor)
{
auto view_registry = meter_context_->GetViewRegistry();
auto ctx = meter_context_.lock();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Check if ctx is valid here before using?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

@esigo esigo enabled auto-merge (squash) August 11, 2022 14:20
@esigo esigo merged commit 4cf41c4 into open-telemetry:main Aug 11, 2022
@esigo esigo deleted the metrics-context-circular-reference branch August 11, 2022 20:08
yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 2022
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.

metrics memory leak because of circular reference

2 participants