Implement the keyed-attributes proposal by jkwatson · Pull Request #1631 · open-telemetry/opentelemetry-java

jkwatson · 2020-09-09T21:03:41Z

This implements the proposal in #1580 .

Many apologies for the size of this PR. The only thing I can say in my defense is that it removes a net of over 1100 lines of code.
Most of the changes are to test code, where attributes are created extensively. The actual change to the API surface is remarkably small.

Resolves #1580

codecov · 2020-09-09T21:20:51Z

Codecov Report

Merging #1631 into master will increase coverage by 0.11%.
The diff coverage is 91.44%.

@@             Coverage Diff              @@
##             master    #1631      +/-   ##
============================================
+ Coverage     86.68%   86.79%   +0.11%     
+ Complexity     1402     1384      -18     
============================================
  Files           164      162       -2     
  Lines          5542     5340     -202     
  Branches        553      546       -7     
============================================
- Hits           4804     4635     -169     
+ Misses          542      509      -33     
  Partials        196      196

Impacted Files	Coverage Δ	Complexity Δ
...ain/java/io/opentelemetry/trace/DefaultTracer.java	`97.61% <ø> (ø)`	`5.00 <0.00> (ø)`
api/src/main/java/io/opentelemetry/trace/Span.java	`100.00% <ø> (ø)`	`0.00 <0.00> (ø)`
.../opentelemetry/exporters/otlp/ResourceAdapter.java	`100.00% <ø> (ø)`	`1.00 <0.00> (ø)`
...a/io/opentelemetry/exporters/otlp/SpanAdapter.java	`95.83% <ø> (ø)`	`19.00 <0.00> (ø)`
...o/opentelemetry/extensions/trace/MessageEvent.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
.../main/java/io/opentelemetry/sdk/trace/Sampler.java	`100.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...ava/io/opentelemetry/opentracingshim/SpanShim.java	`44.11% <25.00%> (+1.26%)`	`14.00 <0.00> (ø)`
...opentelemetry/opentracingshim/SpanBuilderShim.java	`53.68% <50.00%> (ø)`	`22.00 <0.00> (ø)`
...java/io/opentelemetry/common/AttributeKeyImpl.java	`57.14% <57.14%> (ø)`	`7.00 <7.00> (?)`
...ava/io/opentelemetry/exporters/jaeger/Adapter.java	`97.00% <88.88%> (-0.35%)`	`19.00 <1.00> (-4.00)`
... and 20 more

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 c571e96...40f54ae. Read the comment docs.

bogdandrutu

Not sure how much I like it, but I can see the benefits. Can we discuss this Friday during the sig meeting?

bogdandrutu · 2020-09-10T16:30:28Z

     *
     * @return this Builder
     */
    public Builder setAttribute(String key, long value) {


Should this change to LongKey, long value?

My thought was, since these are already type-safe, to keep the easy option in the API, for cases where people don't want to directly use the new keys. The method that takes the generic key should cover the particular, and would end up being the same method signature as this if we change the first parameter to LongKey.

Yeah I don't think we need to change these methods to use the keys. We could consider removing these methods - I'm on the fence on how useful these are for users. Keys will always have a significant performance benefit compared to creating a wrapper key every time this is invoked, so removing them reduces the chances of a user not realizing that.

At the very least, I was thinking we should add some strongly worded language to the javadoc about preferring to use the method that takes the Key.

added some javadoc to the relevant methods.

anuraaga

Generally looks good to me

anuraaga · 2020-09-11T07:55:50Z

+
+  @Override
+  public String toString() {
+    return "AttributeKeyImpl{" + "key='" + key + '\'' + '}';


Suggested change

return "AttributeKeyImpl{" + "key='" + key + '\'' + '}';

return key;

I think just the key is a better toString for keys

I'm on the fence. It will certainly read better, but I wonder if it could be misleading in tests if the failure happens when you're trying to compare to a raw String, and the toStrings would look identical.

Admittedly I'm biased because instrumentation tests are groovy and would be able to use the key itself in assertions instead of calling a method. This is not a strong enough reason to pick one or another though so ok with either 😃

wait...spock uses the toString to do assertions??? That sounds like a terrible idea.

anuraaga · 2020-09-11T08:04:12Z

     *
     * @return this Builder
     */
    public Builder setAttribute(String key, long value) {


Yeah I don't think we need to change these methods to use the keys. We could consider removing these methods - I'm on the fence on how useful these are for users. Keys will always have a significant performance benefit compared to creating a wrapper key every time this is invoked, so removing them reduces the chances of a user not realizing that.

carlosalberto · 2020-09-11T12:28:02Z

Overall good - I was worried at first at the beginning because we lost some type safety, but if the memory footprint really improves (and maybe we also help the Instrumentation?), I'm ok with it definitely.

thisthat · 2020-09-14T11:45:07Z

@jkwatson thank you for the huge amount of work put into this PR! In the last SIG meeting, you said there are fewer allocations with this approach! Do you have the JMH number at hand?

iNikem

I reviewed about half of the PR and in do agree with this change in general :)

iNikem · 2020-09-13T13:52:36Z

+
+  //////////////////////////////////
+  // IMPORTANT: the equals/hashcode/compareTo *only* include the key, and not the type,
+  // so that de-duping of attributes is based on the key, and not also based on the type.


Why? Can there be attributes with the same key but different types?

Well... what would that mean, exactly? Certainly with the previous implementation, you get only one value per key. Having multiple values per string-key would be very odd to have to deal with in an Exporter, I think.

My though was that if there is NO two attributes with the same key but different type, why it is important to include only key, but not type, into equals/hashcode/compareTo?

Something has to make the replacement of one keyed-value with another (of a different type). This is the implementation that makes that happen. The de-duping relies on the equals/hashcode not including the type.

jkwatson · 2020-09-14T15:38:19Z

@jkwatson thank you for the huge amount of work put into this PR! In the last SIG meeting, you said there are fewer allocations with this approach! Do you have the JMH number at hand?

I do. I've been using the SpanPipelineBenchmark for this test, which does exercise attributes, although maybe or maybe not in a super realisitc way. Here is the after/befpre for that (note: don't pay much attention to the timing numbers here; only the allocation numbers should be useful, since I ran these on my laptop):

keyed_attributes:
SpanPipelineBenchmark.runThePipeline_05Threads                                   thrpt    5  3035.071 ± 187.490  ops/ms
SpanPipelineBenchmark.runThePipeline_05Threads:·gc.alloc.rate                    thrpt    5  5427.078 ± 336.116  MB/sec
SpanPipelineBenchmark.runThePipeline_05Threads:·gc.alloc.rate.norm               thrpt    5  2816.001 ±   0.001    B/op
SpanPipelineBenchmark.runThePipeline_05Threads:·gc.churn.PS_Eden_Space           thrpt    5  5408.387 ± 820.544  MB/sec
SpanPipelineBenchmark.runThePipeline_05Threads:·gc.churn.PS_Eden_Space.norm      thrpt    5  2805.580 ± 276.429    B/op
SpanPipelineBenchmark.runThePipeline_05Threads:·gc.churn.PS_Survivor_Space       thrpt    5     0.187 ±   0.114  MB/sec
SpanPipelineBenchmark.runThePipeline_05Threads:·gc.churn.PS_Survivor_Space.norm  thrpt    5     0.097 ±   0.053    B/op
SpanPipelineBenchmark.runThePipeline_05Threads:·gc.count                         thrpt    5    60.000            counts
SpanPipelineBenchmark.runThePipeline_05Threads:·gc.time                          thrpt    5    46.000                ms

Main : 9/11/2020:
SpanPipelineBenchmark.runThePipeline_05Threads                                   thrpt    5  2946.739 ± 165.970  ops/ms
SpanPipelineBenchmark.runThePipeline_05Threads:·gc.alloc.rate                    thrpt    5  5396.947 ± 277.886  MB/sec
SpanPipelineBenchmark.runThePipeline_05Threads:·gc.alloc.rate.norm               thrpt    5  2888.001 ±   0.001    B/op
SpanPipelineBenchmark.runThePipeline_05Threads:·gc.churn.PS_Eden_Space           thrpt    5  5431.879 ± 795.503  MB/sec
SpanPipelineBenchmark.runThePipeline_05Threads:·gc.churn.PS_Eden_Space.norm      thrpt    5  2907.242 ± 467.967    B/op
SpanPipelineBenchmark.runThePipeline_05Threads:·gc.churn.PS_Survivor_Space       thrpt    5     0.245 ±   0.290  MB/sec
SpanPipelineBenchmark.runThePipeline_05Threads:·gc.churn.PS_Survivor_Space.norm  thrpt    5     0.131 ±   0.157    B/op
SpanPipelineBenchmark.runThePipeline_05Threads:·gc.count                         thrpt    5    63.000            counts
SpanPipelineBenchmark.runThePipeline_05Threads:·gc.time                          thrpt    5    48.000                ms

iNikem · 2020-09-15T05:04:17Z

Am I reading it right, that this PR reduces allocation rate by 3%? Somehow I expected more :(

Oberon00 · 2020-09-15T11:43:46Z

+
+    AttributeKeyImpl<?> that = (AttributeKeyImpl<?>) o;
+
+    return getKey() != null ? getKey().equals(that.getKey()) : that.getKey() == null;


Use https://docs.oracle.com/javase/7/docs/api/java/util/Objects.html#equals(java.lang.Object,%20java.lang.Object)?

Suggested change

return getKey() != null ? getKey().equals(that.getKey()) : that.getKey() == null;

return Objects.equals(getKey(), that.getKey());

We can't use that in the API, due to java 7/android

This API is definititely in Java 7, check the link I posted 😃

Oberon00 · 2020-09-15T11:44:55Z

+
+  @Override
+  public int compareTo(AttributeKey o) {
+    return getKey().compareTo(o.getKey());


Why no null check here? Use Objects.compare?

For some reason I thought we couldn't use that with java 7/android. Let me take a look.

updated to be null-safe

carlosalberto · 2020-09-15T13:35:32Z

Somehow I expected more :(

If this helps Auto-Instrumentation, that's a big win already ;)

jkwatson · 2020-09-15T14:34:05Z

Am I reading it right, that this PR reduces allocation rate by 3%? Somehow I expected more :(

Recall that the benchmark I'm running is creating one of each type of attribute. Only the String attribute has an allocation improvement in this scheme. I would guess that, in real instrumentation, most of the attributes are String-typed, and hence the allocation rate gain would be proportionally much better.

jkwatson · 2020-09-16T05:47:41Z

@carlosalberto @bogdandrutu This is good to go. Any last thoughts/concerns before we merge it?

…r Attributes.

Move the key creators to a AttributesKeys class. Hide the implementation classes and replace with interface use.

…ions.

Oberon00 · 2020-09-19T11:15:09Z

Hi, it would be nice if you could merge this PR mostly as-is without relying on Java 8 features (features for newer android API levels are fine), as we would like to include this (hopefully last) major change to the tracing API in our Dynatrace fork.

jkwatson · 2020-09-19T14:57:25Z

Hi, it would be nice if you could merge this PR mostly as-is without relying on Java 8 features (features for newer android API levels are fine), as we would like to include this (hopefully last) major change to the tracing API in our Dynatrace fork.

I'm planning on merging first thing on Monday.

jkwatson · 2020-09-21T15:52:35Z

Merging!

jkwatson added the API API related issues label Sep 9, 2020

jkwatson marked this pull request as ready for review September 9, 2020 21:30

jkwatson requested review from anuraaga, arminru, bogdandrutu, carlosalberto, pavolloffay, thisthat and tylerbenson as code owners September 9, 2020 21:30

bogdandrutu reviewed Sep 10, 2020

View reviewed changes

jkwatson mentioned this pull request Sep 10, 2020

initial commit of logging support extension #1524

Merged

anuraaga reviewed Sep 11, 2020

View reviewed changes

jkwatson mentioned this pull request Sep 11, 2020

Add FaaS attributes. #1634

Merged

jkwatson force-pushed the keyed_attribute_step_2 branch from 78de78f to 3dfeff1 Compare September 11, 2020 19:26

anuraaga reviewed Sep 12, 2020

View reviewed changes

Comment thread api/src/main/java/io/opentelemetry/common/Attributes.java Outdated

Comment thread api/src/main/java/io/opentelemetry/common/AttributesKeys.java

thisthat reviewed Sep 14, 2020

View reviewed changes

Comment thread api/src/main/java/io/opentelemetry/common/AttributeConsumer.java

Comment thread api/src/main/java/io/opentelemetry/common/ImmutableKeyValuePairs.java

iNikem approved these changes Sep 14, 2020

View reviewed changes

jkwatson force-pushed the keyed_attribute_step_2 branch from 35c1b9b to 9977762 Compare September 14, 2020 16:32

anuraaga approved these changes Sep 14, 2020

View reviewed changes

thisthat approved these changes Sep 15, 2020

View reviewed changes

Oberon00 reviewed Sep 15, 2020

View reviewed changes

jwatson and others added 18 commits September 17, 2020 08:00

Make AttributesMap not extend map, and restore the get method name fo…

03db6d1

…r Attributes.

key class and implementation

ef6ba96

key class and implementation

e72fdb0

The code compiles

2b6a072

little tweaks

e572381

some cleanup, tests are passing

230de0a

remove AttributeValue and clean up misc. references to it.

fc11544

clean up some unused bits

01c89f1

add a TODO

a7dbe16

a bit of cleanup; fix a todo

55e22bf

update for changes from the main branch

269e259

Change method to get the underlying key.

1155c27

Move the key creators to a AttributesKeys class. Hide the implementation classes and replace with interface use.

use autovalue for the key implementations

e1ab103

fix javadoc issues

cac0ae5

update benchmark keys, and a few tweaks from PR review

d98eb2a

Add javadoc notes to encourage using the lower-overhead attribute opt…

77f0fb5

…ions.

Add javadoc clarifying subclass responsibilities around empty keys.

8f1f871

make the compareTo on AttributeKeyImpl null-safe, for extra safety

dc86d82

jkwatson force-pushed the keyed_attribute_step_2 branch from 9aceb85 to dc86d82 Compare September 17, 2020 15:01

tylerbenson approved these changes Sep 18, 2020

View reviewed changes

Oberon00 mentioned this pull request Sep 21, 2020

Drop Java 7 support for both the API and SDK #1649

Closed

jkwatson added 2 commits September 21, 2020 07:48

Merge branch 'master' into keyed_attribute_step_2

529c15f

fix formatting

40f54ae

jkwatson merged commit bf52c0a into open-telemetry:master Sep 21, 2020

jkwatson deleted the keyed_attribute_step_2 branch September 21, 2020 15:52

This was referenced Sep 21, 2020

Proposal: Remove AttributeValue from the public interface. #1081

Closed

SemanticAttribute setters are not straightforward to use with SpanBuilder #1075

Closed

	return "AttributeKeyImpl{" + "key='" + key + '\'' + '}';
	return key;


		AttributeKeyImpl<?> that = (AttributeKeyImpl<?>) o;

		return getKey() != null ? getKey().equals(that.getKey()) : that.getKey() == null;

	return getKey() != null ? getKey().equals(that.getKey()) : that.getKey() == null;
	return Objects.equals(getKey(), that.getKey());

Conversation

jkwatson commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bogdandrutu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anuraaga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carlosalberto commented Sep 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thisthat commented Sep 14, 2020

Uh oh!

iNikem left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jkwatson commented Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iNikem commented Sep 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosalberto commented Sep 15, 2020

Uh oh!

jkwatson commented Sep 9, 2020 •

edited

Loading

codecov bot commented Sep 9, 2020 •

edited

Loading

jkwatson commented Sep 14, 2020 •

edited

Loading

Oberon00 commented Sep 19, 2020 •

edited

Loading