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

Add AllLabels Batcher and install default Barcher for Counters #831

Merged
merged 2 commits into from
Feb 20, 2020
Merged

Add AllLabels Batcher and install default Barcher for Counters #831

merged 2 commits into from
Feb 20, 2020

Conversation

bogdandrutu
Copy link
Member

No description provided.

@bogdandrutu bogdandrutu mentioned this pull request Feb 7, 2020
Merged
@bogdandrutu bogdandrutu force-pushed the newbounds branch 2 times, most recently from f912b33 to 68c76f6 Compare February 7, 2020 18:32
@bogdandrutu bogdandrutu force-pushed the newbounds branch 2 times, most recently from a11d370 to fc4fcf1 Compare February 7, 2020 22:15
@codecov-io
Copy link

codecov-io commented Feb 7, 2020 •
edited
Loading

Codecov Report

Merging #831 into master will increase coverage by 0.38%.
The diff coverage is 84.9%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #831      +/-   ##
============================================
+ Coverage      82.3%   82.68%   +0.38%     
- Complexity      839      845       +6     
============================================
  Files           111      111              
  Lines          2882     2934      +52     
  Branches        253      257       +4     
============================================
+ Hits           2372     2426      +54     
+ Misses          399      397       -2     
  Partials        111      111
Impacted Files Coverage Δ Complexity Δ
.../io/opentelemetry/sdk/metrics/data/MetricData.java 100% <ø> (ø) 2 <0> (ø) ⬇️
.../io/opentelemetry/sdk/metrics/AbstractMeasure.java 88% <0%> (-3.67%) 9 <0> (ø)
...io/opentelemetry/sdk/metrics/AbstractObserver.java 84.61% <0%> (-3.39%) 9 <0> (ø)
.../io/opentelemetry/sdk/metrics/AbstractCounter.java 94.73% <100%> (+3.07%) 13 <4> (+4) ⬆️
...o/opentelemetry/sdk/metrics/view/Aggregations.java 52.63% <25%> (-3.26%) 2 <0> (ø)
...in/java/io/opentelemetry/sdk/metrics/Batchers.java 91.89% <90.62%> (-8.11%) 2 <1> (+1)
...elemetry/sdk/trace/export/BatchSpansProcessor.java 89.65% <0%> (-1.73%) 7% <0%> (ø)
...try/sdk/metrics/AbstractInstrumentWithBinding.java 76% <0%> (+8%) 6% <0%> (+1%) ⬆️
...io/opentelemetry/sdk/metrics/DoubleCounterSdk.java 95.83% <0%> (+20.83%) 5% <0%> (ø) ⬇️
...a/io/opentelemetry/sdk/metrics/LongCounterSdk.java 95.83% <0%> (+20.83%) 5% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b84bf39...22bd1e7. Read the comment docs.

@bogdandrutu bogdandrutu force-pushed the newbounds branch 3 times, most recently from 8728040 to a756728 Compare February 10, 2020 17:17
@bogdandrutu bogdandrutu mentioned this pull request Feb 10, 2020
Merged
@bogdandrutu bogdandrutu force-pushed the newbounds branch 2 times, most recently from 15984ec to da8de26 Compare February 10, 2020 18:10
This was referenced Feb 11, 2020
Merged
Merged
@bogdandrutu bogdandrutu force-pushed the newbounds branch 4 times, most recently from 33e845d to d93ffa3 Compare February 14, 2020 01:36
jkwatson
jkwatson reviewed Feb 14, 2020
View reviewed changes
sdk/src/main/java/io/opentelemetry/sdk/metrics/AggregatorMap.java Outdated
private final Resource resource;
private final InstrumentationLibraryInfo instrumentationLibraryInfo;
private final AggregatorFactory aggregatorFactory;
private final Map<Map<String, String>, Aggregator> aggregatorMap;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can use LabelSetSdk as the key to this map, since it has a well-defined equals/hashcode via autovalue.

@thisthat thisthat mentioned this pull request Feb 14, 2020
Closed
6 tasks
@bogdandrutu bogdandrutu force-pushed the newbounds branch 8 times, most recently from b004484 to 1361d42 Compare February 18, 2020 19:23
@bogdandrutu bogdandrutu mentioned this pull request Feb 18, 2020
Merged
@bogdandrutu bogdandrutu force-pushed the newbounds branch 2 times, most recently from c31878c to 3871f6a Compare February 19, 2020 22:23
@bogdandrutu
Bound registry work in progress
5a8bce6 
Signed-off-by: Bogdan Cristian Drutu <[email protected]>
@bogdandrutu bogdandrutu changed the title Bound registry work in progress Add AllLabels Batcher and install default Barcher for Counters Feb 19, 2020
@bogdandrutu
Copy link
Member Author

@jkwatson PTAL, after this PR is merged we have Counters working :)

jkwatson
jkwatson reviewed Feb 19, 2020
View reviewed changes
sdk/src/main/java/io/opentelemetry/sdk/metrics/Batchers.java
public final List<MetricData> completeCollectionCycle() {
List<Point> points = new ArrayList<>(aggregatorMap.size());
long epochNanos = clock.now();
for (Map.Entry<Map<String, String>, Aggregator> entry : aggregatorMap.entrySet()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a risk of a ConcurrentModificationException happening during this iteration, since the map can be mutated during this method call.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is guaranteed that only one thread calls into a Batcher collection at a time by the collectLock in the AbstractInstrumentWithBindings

Copy link
Contributor

Choose a reason for hiding this comment

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

please add a comment in here to that effect. thanks!

jkwatson
jkwatson approved these changes Feb 19, 2020
View reviewed changes
Copy link
Contributor

@jkwatson jkwatson left a comment

Choose a reason for hiding this comment

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

A comment about thread safety, and this should be good to start playing with!

@bogdandrutu
Copy link
Member Author

@jkwatson done

@bogdandrutu bogdandrutu merged commit 9aa6d1e into open-telemetry:master Feb 20, 2020
@bogdandrutu
Copy link
Member Author

Please feel free to file issues or PRs against the code merged here.

@bogdandrutu bogdandrutu deleted the newbounds branch February 20, 2020 00:24
DotSpy pushed a commit to DotSpy/opentelemetry-java that referenced this pull request Feb 21, 2020
@bogdandrutu @DotSpy
Add AllLabels Batcher and install default Barcher for Counters (open-…
91d14ae 
…telemetry#831)

* Bound registry work in progress

Signed-off-by: Bogdan Cristian Drutu <[email protected]>

* Add comment about protecting a collection cycle by a lock

Signed-off-by: Bogdan Cristian Drutu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thisthat thisthat thisthat left review comments

@jkwatson jkwatson jkwatson approved these changes

@arminru arminru Awaiting requested review from arminru

@carlosalberto carlosalberto Awaiting requested review from carlosalberto

@pavolloffay pavolloffay Awaiting requested review from pavolloffay

@songy23 songy23 Awaiting requested review from songy23

@tylerbenson tylerbenson Awaiting requested review from tylerbenson

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

Successfully merging this pull request may close these issues.
4 participants
@bogdandrutu @codecov-io @jkwatson @thisthat