Implement thread.id and thread.name semantic attributes

[mateuszrzeszutek]

This small PR implements two new semantic attributes, added in open-telemetry/opentelemetry-specification#789

[anuraaga]

Thanks!

[codecov]

Codecov Report

Merging #1554 into master will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1554      +/-   ##
============================================
+ Coverage     91.54%   91.61%   +0.06%     
  Complexity      976      976              
============================================
  Files           116      116              
  Lines          3514     3517       +3     
  Branches        304      304              
============================================
+ Hits           3217     3222       +5     
+ Misses          204      203       -1     
+ Partials         93       92       -1     

Impacted Files	Coverage Δ	Complexity Δ
...telemetry/trace/attributes/SemanticAttributes.java	100.00% <100.00%> (ø)	1.00 <0.00> (ø)
...y/sdk/metrics/aggregator/DoubleMinMaxSumCount.java	100.00% <0.00%> (+3.92%)	6.00% <0.00%> (ø%)

---

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 2659726...59a4c21. Read the comment docs.

[iNikem]

Shouldn't io.opentelemetry.sdk.trace.SpanBuilderSdk#startSpan be changed as well to actually set those attributes?

[mateuszrzeszutek]

Shouldn't io.opentelemetry.sdk.trace.SpanBuilderSdk#startSpan be changed as well to actually set those attributes?

That's a great idea -- I initially planned to set them somewhere in BaseTracer#startSpan in the instrumentation project, but this looks like a better place.

[Oberon00]

I don't think this should be baked into the core SDK. Not all users will want it, and it can be added just fine in a SpanProcessor's OnStart. E.g. you would call my.extension.module.ThreadStartAnnotationSpanProcessor.enable()

[iNikem]

Why should we give users in this particular place a control over semantic conventions? I can understand "there should not be semantic conventions in SDK at all", but not some specific handling of this convention.

Also, I find it puzzling that so much functionality across different issues is being proposed to be handled by SpanProcessor. Spec says

Each processor registered on TracerProvider is a start of pipeline that consist of span processor and optional exporter

I read it as there is no guarantee that span modifications of any single SpanProcessor will reach all exporters.

[mateuszrzeszutek]

it can be added just fine in a SpanProcessor's OnStart

Unfortunately in can't be done this way in current Java SDK: SpanProcessor#onStart() receives an immutable ReadableSpan -- even though it should be a R/W instance according to the spec.

[Oberon00]

@iNikem

Why should we give users in this particular place a control over semantic conventions? I can understand "there should not be semantic conventions in SDK at all", but not some specific handling of this convention.

There is no requirement that this semantic convention is added to all spans, nor should there be IMHO.

Each processor registered on TracerProvider is a start of pipeline that consist of span processor and optional exporter

I read it as there is no guarantee that span modifications of any single SpanProcessor will reach all exporters.

Oh good catch, this sentence is very confusing. You could interpret it in such a way that a span instance has to be created per registered SpanProcessor. That's not how this is meant however (at least for OnStart), if you look at the git blame / PR behind it. This was even discussed with exactly "thread name" as example 😄 Please read open-telemetry/opentelemetry-specification#338 (comment) and before. I will create an issue/PR to clarify or remove this sentence.

[Oberon00]

@mateuszrzeszutek

Unfortunately in can't be done this way in current Java SDK: SpanProcessor#onStart() receives an immutable ReadableSpan -- even though it should be a R/W instance according to the spec.

That should be an easy PR. According to @bogdandrutu's comments at open-telemetry/opentelemetry-specification#669 (comment), he would like to see Span added as additional parameter for OnStart (of course the same object instance will be behind both the Span and the ReadableSpan).

[mateuszrzeszutek]

Okay, so we have two options here:

a) Add thread.id and thread.name in SpanBuilderSdk

b) Implement a SpanProcessor that adds them. That requires:

1. Changing the OnStart spec to that it explicitly states that changes applied here will be visible in all other processors that run later; and that a Read/write span may actually consist of two parameters;
2. Adding a second Span param to SpanProcessor#onStart() so that it supports both reading and modifying a span (this indeed seems to be an easy change);
3. Implementing a SpanProcessor in this PR;
4. Using the new processor in the Java agent; possibly guarded with a configuration flag.

Again, since I'm new to OTel world I'd like to rely on your judgement here.

[iNikem]

Option three is just implement attribute setting in instrumentation repo as you planned at the beginning.

[iNikem]

You could interpret it in such a way that a span instance has to be created per registered SpanProcessor.

No, my concern is different. But I just realised that my concern is about onEnd, not onStart. So we can ignore it, I managed to clarify my own confusion to myself :)

[Oberon00]

@mateuszrzeszutek

...
b) Implement a SpanProcessor that adds them. That requires:

1. Changing the OnStart spec to that it explicitly states that changes applied here will be visible in all other processors that run later; and that a Read/write span may actually consist of two parameters;

2. Adding a second Span param to SpanProcessor#onStart() so that it supports both reading and modifying a span (this indeed seems to be an easy change);

...

These two steps need to be done anyway (best in a separate PR) to make opentelemetry-java conform to the spec.

[jkwatson]

My 2 cents: we shouldn't have the SDK add these attributes automatically. We need to be very careful when we add attributes, because that can translate into data-storage cost for users, depending on their backend. And, if the user has zero control over that, it would be a bad thing.