Add SpanData.Builder to allow modifying the information in a SpanData…

[anuraaga]

…, for example in an Exporter.

This is an approach that adds toBuilder to SpanData instead of exposing a DelegatingSpanData wrapper since it seemed to have some more votes. I found we actually already have a similar class, TestSpanData, only for tests. So I made that explicit by moving TestSpanData to a separate testing module instead of in our codebase and just delegate to a newly restored SpanDataImpl in tests.

A lot of the pain in this approach comes from supporting both the types of SpanData, one that delegates to the span and one that contains all the information after mutation. I was sad that I couldn't seem to find a way to have AutoValue generate equals for me in a way that supports the base interface, meaning this is almost as much maintainance burden as a wrapper class (where I would have to define equals and hashCode in code).

Let me know if you have thoughts, I can also send a version using the delegating pattern if it seems to have merit. Other options are to add some benchmarks of Span.toSpanData that use the delegating vs just copying everything and if there's only a small difference, we could consolidate on one.

Fixes #1321

[codecov]

Codecov Report

Merging #1502 into master will decrease coverage by 0.51%.
The diff coverage is 60.78%.

@@             Coverage Diff              @@
##             master    #1502      +/-   ##
============================================
- Coverage     92.33%   91.82%   -0.52%     
- Complexity      951      956       +5     
============================================
  Files           117      117              
  Lines          3380     3413      +33     
  Branches        281      302      +21     
============================================
+ Hits           3121     3134      +13     
- Misses          172      173       +1     
- Partials         87      106      +19     

Impacted Files	Coverage Δ	Complexity Δ
...n/java/io/opentelemetry/sdk/trace/SpanWrapper.java	95.65% <0.00%> (-4.35%)	22.00 <0.00> (ø)
...java/io/opentelemetry/sdk/trace/data/SpanData.java	100.00% <ø> (ø)	0.00 <0.00> (ø)
.../java/io/opentelemetry/sdk/trace/SpanDataImpl.java	62.00% <62.00%> (ø)	7.00 <7.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 8d0a478...f9cc04f. Read the comment docs.

[anuraaga]

/cc @iNikem

[iNikem]

Yeah, I saw it, but was yet unable to formulate my feedback :) I feel like I don't like it, but cannot give anything constructive yet. Will ponder it more.

[jkwatson]

One of the bummers with this approach is that every time you want to modify the SpanData, you end up copying all of it, even if all you care about it modifying one particular field. If you had a abstract wrapper class, you could, in your constructor, alter or augment only the pieces that you cared about, keeping the delegate implementation in place for all other fields, potentially saving a lot of allocation overhead.

[iNikem]

@jkwatson Could you? How can you alter or augment ReadableAttributes? This is the biggest mystery to me in this whole business.

[jkwatson]

@jkwatson Could you? How can you alter or augment ReadableAttributes? This is the biggest mystery to me in this whole business.

You could read them (they are Readable, after all), and create your own Attributes to return, based on them, with some kind of augmentation. Maybe I'm missing the confusion?

[anuraaga]

It sounds like people want to see the wrapper approach as well so will send a PR for comparison :)

[iNikem]

@jkwatson Could you? How can you alter or augment ReadableAttributes? This is the biggest mystery to me in this whole business.

You could read them (they are Readable, after all), and create your own Attributes to return, based on them, with some kind of augmentation. Maybe I'm missing the confusion?

I think that my brain refuses to think about a solution which involves iterating over attributes and copying them one by one into new attributes instance :)

[anuraaga]

@iNikem Yep attributes are copied, if we find this to be a significant performance issue we'd need to add ChainedAttributes 😿 Is this Java or Scala? :D

[iNikem]

Wait, I have read https://github.com/google/auto/blob/master/value/userguide/builders-howto.md#to_builder and think we have overcomplicated things.

What if we make SpanWrapper public, add Builder to add and add public abstract Builder toBuilder(); method to it?

Then the only question that will remain is how to add new attributes. If we make SpanData.attributes() to return io.opentelemetry.common.Attributes instead of ReadableAttributes, then AutoValue magic will work as well.

@jkwatson @anuraaga WDYT?

[anuraaga]

@iNikem I'd have to try to be sure, but I'm not expecting AutoValue's Builder to work in this case. There's only one AutoValue implementation ,SpanWrapper - there'd be no way for that Builder to store the value if a user called setStatus on the builder for example. SpanWrapper is a class with a field of type RecordEventsReadableSpan (not a public class), List<Link>, List<Event> so that's what the builder can make but can't generate something that allows arbitrary mutation.

It also only partly solves the copying allocation issue that @jkwatson refers to I think #1502 (comment) I think you can see when comparing to #1507

[iNikem]

there'd be no way for that Builder to store the value if a user called setStatus on the builder for example

I don't understand this part. There is already AutoValue_SpanWrapper.status field, no?

Here is my attempt

  @AutoValue.Builder
  public abstract static class Builder {
    abstract Builder setDelegate(RecordEventsReadableSpan value);
    abstract Builder setTotalAttributeCount(int value);
    abstract Builder setTotalRecordedEvents(int value);

    public abstract Builder setResolvedLinks(List<Link> value);
    public abstract Builder setResolvedEvents(List<Event> value);
    public abstract Builder setAttributes(Attributes value);
    public abstract Builder setStatus(Status value);
    public abstract SpanWrapper build();
  }

[anuraaga]

@iNikem Apologies, status was a bad example. Resource (or any of the fields we delegate for) are what I was thinking about.

[iNikem]

Yes, only subset of SpanData will be modifiable. I don't think this is a bad thing. Although we may want to make Resource modifiable, I doubt we want to change traceId :)

[anuraaga]

I think the delegation of SpanWrapper is just an implementation detail for optimization, but the API is SpanData just a simple POJO. I think it's not idiomatic to present a toBuilder that doesn't expose the entire POJO, it doesn't really happen in Java.

Also, FWIW I have mutated the trace ID before in an exporter

https://github.com/aws-samples/aws-xray-sdk-with-opentelemetry-sample/blob/both-otel-and-xray/frontend/src/main/java/com/softwareaws/xray/examples/ZipkinTracingConfiguration.java#L84

It's not a normal use case at all but helped a lot and I was glad it was allowed. If we're giving a user the ability to mutate a SpanData I think it's the most intuitive API to allow mutation of all of it - it's fairly advanced either way.

[anuraaga]

So instead of digging too much into AutoValue, let's not be constrained by it, if there's general direction on an API I'll try to clean up duplication, etc that I can.

AFAICT the proposed APIs right now are
Full builder
Partial builder
Delegating wrapper

And open to other ideas too :)

[jkwatson]

I guess after seeing both implementations, my original thoughts still stand. Anyone who want to modify the SpanData that is handed to them can do it in whatever way makes sense for them. Our providing a wrapper, or a builder, or whatever doesn't add a huge value in the general case.

It sounds like @iNikem is more concerned with the overall immutability that we've built into the our data structures, that we've done explicitly to enable performance optimizations.

[iNikem]

@jkwatson do you propose that every "anyone" who wants to do that should create their own implementation of SpanData? There is no public implementation atm. So everyone will duplicate the same code over and over again?

[jkwatson]

@jkwatson do you propose that every "anyone" who wants to do that should create their own implementation of SpanData? There is no public implementation atm. So everyone will duplicate the same code over and over again?

If all you want is a simple wrapper, it seems reasonable to wait until we have one implementation in the real world before we preemptively put it into the SDK itself as something official. If this is needed in the instrumentation project, how about starting by creating it there, and getting all the kinks worked out, and then consider moving it into the official SDK?

[iNikem]

Yeah, I suppose I will go this way :(

[anuraaga]

Our providing a wrapper, or a builder, or whatever doesn't add a huge value in the general case.

They seem fairly general to me (at least full builder or delegator) - they make it very simple to modify just a small part of SpanData.

I don't mind the idea of developing it in instrumentation repo, but a couple points I want to bring up are

• As Add a DelegatingSpanData that can be used to wrap SpanData with different data. #1507 demonstrates, it's a huge amount of work to write a proper wrapper. Reducing the difficulty of this does seem important for the SDK. The instrumentation repo can do it, but it doesn't actually manage the exporters so it's quite a strange line to have export helpers there

• We already have TestSpanData which is actually already the full builder in most aspects. It's currently a questionable class, since it's intended for use in tests but is actually in the SDK package itself, and it is far less useful even for tests than it should since it can't be compared with equals to SpanWrapper which the SDK generates, mostly due to AutoValue's limitation.

• Providing toBuilder to an immutable POJO is pretty common. I definitely agree with minimizing public API as a general principle, but providing full POJOs proactively doesn't seem that bad

Actually I'm more happy of trying to solve the second point in this PR than even the first :P Which is a reason I slightly favor this one over #1507.

@jkwatson Let me know if you have any thoughts on these points, especially what to do with TestSpanData. If it's still a preference to not add these, I'll close the PRs :)

[jkwatson]

I 100% agree that TestSpanData should be moved out of the SDK proper, and I'm ok with putting one of the two PRs into the SDK. I personally prefer the wrapper, since it will encourage fewer allocations. I'm not strongly opposed to any of it, but I also don't feel strongly that it actually adds a lot to the SDK, since writing a wrapper class is quite trivial, IMO.

[anuraaga]

So extracted the point that had concensus into #1512 :)

I do still think we should add something since maintaining a wrapper is hard for a user if we add methods to the SpanData interface. I don't have a strong preference for either builder or wrapper so am leaning with #1507 if it's less controversial - @iNikem do you have any thoughts?

[iNikem]

I am interested in this whole discussion because I have to make a POC Splunk specific distribution where I want to add some attributes. So any solution that will allow me to do that is good for me atm. Including me following John advice and just writing it myself in my own codebase :)