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.

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

Add the Asynchronous and Synchronous state packages for review LS-29754 #173

Merged
merged 10 commits into from
Jun 7, 2022

Conversation

jmacd
Copy link
Member

@jmacd jmacd commented May 23, 2022

Description:
The internal/syncstate and internal/asyncstate package contain the adapters used by the MeterProvider to the viewstate package's compiler. The syncstate package, in particular, handles the hot code path used by synchronous instruments.

Link to tracking Issue:
LS-29760 describes the overall goal; this is one part.

Testing:
End-to-end testing was done. The alt_metrics_sdk/development branch has a assembled copy of this code. Depends on various Lightstep-side PRs merging to see the exponential histograms work.

@jmacd
Copy link
Member Author

jmacd commented Jun 6, 2022

Reviewers: as discussed last week, there was still a TODO about a race condition; the synchronous-instrument delta-temporality collector was not correctly synchronized for deletion to occur from its map. This could have been repaired in at least two ways; I chose the one that IMO is better, but this is subtle. This change is shown in the following commit:

54ac1b8

This needs more documentation and can use further testing, however it resolves the races and TODO. The design is as follows:

  1. No change in the synchronous instrument hot path
  2. Concurrent collection from synchronous instruments happens concurrently, up and through the accumulators
  3. Allocators are (continue to be) added to the collector's map under a lock and released after SnapshotAndProcess w/o the lock
  4. SnapshotAndProcess() takes a new boolean argument, saying if it is the final snapshot. This lets the collectors take advantage of the syncstate reference counting mechanism.
  5. Collection takes the same lock (blocking new accumulators) and synchronously moves the data into the output
  6. Then, as a special case, after the synchronous move, the zero value is skipped
  7. If there was a zero value and the refcount is zero (since the lock is held), safe to remove the entry from the map.

Copy link
Contributor

@jaronoff97 jaronoff97 left a comment

Choose a reason for hiding this comment

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

A few comments, but nothing blocking. 💯

@jmacd jmacd merged commit 2bd0980 into alt_metrics_sdk/reviewed Jun 7, 2022
@jmacd jmacd deleted the alt_metrics_sdk/review_internalstate branch June 7, 2022 19:31
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.

3 participants