Skip to content

Fix observable attributes drop - #2557

Merged
ThomsonTan merged 2 commits into
open-telemetry:mainfrom
lalitb:fix-observable-attributes-drop
Feb 27, 2024
Merged

ThomsonTan merged 2 commits into
open-telemetry:mainfrom
lalitb:fix-observable-attributes-drop

Conversation

@lalitb

@lalitb lalitb commented Feb 27, 2024

Copy link
Copy Markdown
Member

Fixes #2548

Changes

PR #2472 introduced the bug to drop the attributes in Observable Instruments. This PR addresses and corrects that issue. However, there remains an unrelated, longstanding bug affecting filtering for Observable Instruments, which has never been reported. A separate bug report will be filed for that issue.

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

@lalitb
lalitb requested a review from a team February 27, 2024 07:58
void Observe(T value) noexcept override { data_.insert({{}, value}); }
void Observe(T value) noexcept override
{
data_[MetricAttributes{{}, attributes_processor_}] = value;

@lalitb lalitb Feb 27, 2024

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.

This change is unrelated to the bug. Ideally, observing a value with an identical set of attributes should lead to it being overwritten. However, at present, using std::unordered_map::insert() leads to the value being disregarded rather than replaced. The observable callback shouldn't be reporting multiple values for the same attribute-set in single invoke, but fixing it just in case they do so.

nostd::get<nostd::shared_ptr<opentelemetry::metrics::ObserverResultT<double>>>(
observer_result)
->Observe(value_ /*, labelkv */);
->Observe(value_, labels);

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.

Tested the changes by the modified metrics example. More exhaustive tests need to be added for observable instruments, will add them along with the filtering issue fix.

@lalitb

lalitb commented Feb 27, 2024

Copy link
Copy Markdown
Member Author

@marcalff @ThomsonTan @esigo - I believe we need a patch release with the fix, as this is a common use-case and multiple users have reported it (few internally in my company too). I can create one (being culprit) once this is reviewed if you guys agree.

Comment thread sdk/src/metrics/state/observable_registry.cc

@esigo esigo left a comment

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.

LGTM
Thanks for the fix

@ThomsonTan
ThomsonTan merged commit 32196a5 into open-telemetry:main Feb 27, 2024
malkia added a commit to malkia/opentelemetry-cpp that referenced this pull request Feb 27, 2024
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.

Asynchronous metric instruments ignoring attributes

3 participants