Add a AutoConfiguredComponentWrapper SPI #3714

anuraaga · 2021-10-08T05:33:44Z

Also graduates DelegatingSpanData out of incubator.

anuraaga

Not for 1.7

anuraaga · 2021-10-08T05:34:13Z

...ure-spi/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/traces/SpanExporterWrapper.java

+ * interface will be invoked to replace it, commonly with a wrapping {@link SpanExporter} which uses
+ * {@link io.opentelemetry.sdk.trace.data.DelegatingSpanData} to adjust the exported data.
+ */
+public interface SpanExporterWrapper {


Somehow SpanExporterWrapperProvider doesn't seem right, but if the Provider suffix pattern is important, it could maybe be SpanExporterWrappingProvider

This seems like we're building a pipeline tool via the decorator pattern. Does it make more sense to instead expose something like SpanProcessor, but in a more explicit that allows modifying spans? What are the use cases here?

I have two use cases:

Modify or drop certain span attributes using DelegatingSpanData

Drop certain spans all together from Collection<SpanData>

Seems like the job of a span processor. From the spec:

SDK MUST allow users to implement and configure custom processors and decorate built-in processors for advanced scenarios such as tagging or filtering.

I put together a little test demonstration of how to accomplish this with span processors here: https://github.com/jack-berg/opentelemetry-java/blob/filter-span-processor/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/FilterAndEnrichSpanProcessorTest.java#L26-L58

Its definitely possible to replicate that via SPI through the SdkTracerProviderConfigurer. But not very conveniently.

That looks good but can we make SpanProcessor loadable through SPI? E.g. Maybe a SpanProcessorProvider? That will make configuration part easier.

I believe it's not possible to remove attributes in a span processor since the span is not mutable in onEnd. This came up in the pre-1.0 days and it was generally understood that unless Span processor were updated with a mutable end method, Span exporter is the only place to have full control of mutation, especially filtering attributes.

Well timed comment came in too :)

#2077 (comment)

I've added another example to my demo branch that shows how to create a SpanExporter, which delegates to another SpanExporter after filtering out certain attributes: https://github.com/jack-berg/opentelemetry-java/blob/filter-span-processor/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/FilterSpanExporter.java#L30-L61

In this case, it filters out attributes which are not of type string.

anuraaga · 2021-10-08T05:35:11Z

@edwardxia Can you check if this pattern will work fine for you?

edwardxia · 2021-10-08T07:19:06Z

It should work, and indeed this would be much easier to use comparing to custom exporter SPI just for the decorating use case. Thanks!

jkwatson · 2021-10-11T15:40:05Z

docs/apidiffs/current_vs_latest/opentelemetry-sdk-extension-autoconfigure-spi.txt

+	+++  CLASS FILE FORMAT VERSION: 52.0 <- n.a.
+	+++  NEW SUPERCLASS: java.lang.Object
+	+++  NEW METHOD: PUBLIC(+) ABSTRACT(+) io.opentelemetry.sdk.trace.export.SpanExporter wrap(io.opentelemetry.sdk.trace.export.SpanExporter, io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties)


Urk. Did we intend to make this module stable, even though the autoconfigure module isn't stable?

Yeah we had intended the interfaces to go stable when we did the split. We talked about this at the time :)

jkwatson · 2021-10-11T15:47:54Z

...utoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/SpanExporterConfiguration.java

+                  for (SpanExporterWrapper wrapper : wrappers) {
+                    exporter = wrapper.wrap(exporter, config);
+                  }


this makes me nervous, a bit. Are there cases where this arbitrary ordering might break something inadvertently?

We can add an Order method with default returning 0 like we have in other SPIs.

jkwatson · 2021-10-11T15:49:44Z

Hmm. Although this does solve the very narrow use-case at hand, I wonder if we're thinking too small here. If we start providing SPIs like this, I feel like we're going to end up with a giant pile of SPIs that will be hard to figure out what to do with.

Would it make sense to take a step back and figure out a more general solution to the problem of "take something autoconfigured and tweak it a bit", rather than proliferate the SPIs?

anuraaga · 2021-10-12T00:30:19Z

@jkwatson How about a single SPI with multiple methods for each of the autoconfigured pieces, AutoconfiguredComponentWrapper? That could reduce SPI proliferation

jkwatson · 2021-10-12T15:28:54Z

@jkwatson How about a single SPI with multiple methods for each of the autoconfigured pieces, AutoconfiguredComponentWrapper? That could reduce SPI proliferation

I think something like this would be good. default no-op implementations of all of the methods would make it easy to only deal with the ones you want, as well.

…nto span-exporter-wrapper

anuraaga · 2021-10-14T07:35:33Z

...spi/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/AutoConfiguredComponentWrapper.java

+ * automatically created by autoconfiguration, for example a span exporter, implementations of this
+ * interface will be invoked to replace it, resulting in the final used component.
+ */
+public interface AutoConfiguredComponentWrapper {


I have defined this and used in place of my previous proposed SpanExporterWrapper. Let me know if this looks OK and I'll slot it in the rest of the places

codecov · 2021-10-14T08:08:17Z

Codecov Report

Merging #3714 (cadecb5) into main (3d4dbec) will decrease coverage by 0.07%.
The diff coverage is 72.64%.

@@             Coverage Diff              @@
##               main    #3714      +/-   ##
============================================
- Coverage     89.24%   89.16%   -0.08%     
- Complexity     3839     3868      +29     
============================================
  Files           462      465       +3     
  Lines         11969    12085     +116     
  Branches       1163     1181      +18     
============================================
+ Hits          10682    10776      +94     
- Misses          905      914       +9     
- Partials        382      395      +13

Impacted Files	Coverage Δ
...oconfigure/spi/AutoConfiguredComponentWrapper.java	`0.00% <0.00%> (ø)`
...nsion/incubator/trace/data/DelegatingSpanData.java	`82.41% <ø> (ø)`
.../autoconfigure/spi/internal/ComponentWrapping.java	`40.00% <40.00%> (ø)`
...entelemetry/sdk/trace/data/DelegatingSpanData.java	`82.41% <82.41%> (ø)`
...y/sdk/autoconfigure/SpanExporterConfiguration.java	`94.73% <100.00%> (+0.07%)`	⬆️
...telemetry/sdk/trace/export/BatchSpanProcessor.java	`90.78% <0.00%> (+0.70%)`	⬆️
...ntelemetry/sdk/extension/resources/OsResource.java	`90.69% <0.00%> (+4.65%)`	⬆️
...java/io/opentelemetry/sdk/trace/AttributesMap.java	`100.00% <0.00%> (+5.88%)`	⬆️
...metry/sdk/extension/resources/ProcessResource.java	`87.50% <0.00%> (+6.25%)`	⬆️
...elemetry/sdk/extension/resources/HostResource.java	`92.30% <0.00%> (+15.38%)`	⬆️
... and 1 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 3d4dbec...cadecb5. Read the comment docs.

jkwatson · 2021-10-14T17:51:26Z

...spi/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/AutoConfiguredComponentWrapper.java

+public interface AutoConfiguredComponentWrapper {
+
+  /** Wraps a {@link Resource}. */
+  default Resource wrap(Resource resource, ConfigProperties config) {


wrap is an odd name here, since you'd presumably just use the normal merge functionality for Resources, wouldn't you?

I wonder if the names of these methods should be something more generic than wrap, but a good name escapes me right this second. augment? decorate? customize maybe?

Yeah, the more I think about it, the more I like "AutoConfiguredComponentCustomizer" as the SPI name.

anuraaga · 2021-10-23T03:11:36Z

Closing in favor of #3753

Add a SpanExporterWrapper SPI

09241d7

anuraaga requested review from arminru, bogdandrutu, carlosalberto, jack-berg, jkwatson and jsuereth as code owners October 8, 2021 05:33

anuraaga requested a review from a user October 8, 2021 05:33

anuraaga requested review from Oberon00, pavolloffay, thisthat and tylerbenson as code owners October 8, 2021 05:33

anuraaga commented Oct 8, 2021

View reviewed changes

jkwatson reviewed Oct 11, 2021

View reviewed changes

anuraaga added 2 commits October 14, 2021 16:18

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java i…

073b509

…nto span-exporter-wrapper

Wrapper to beat all wrappers

cadecb5

anuraaga marked this pull request as draft October 14, 2021 07:34

anuraaga changed the title ~~Add a SpanExporterWrapper SPI~~ Add a AutoConfiguredComponentWrapper SPI Oct 14, 2021

anuraaga commented Oct 14, 2021

View reviewed changes

jkwatson reviewed Oct 14, 2021

View reviewed changes

anuraaga mentioned this pull request Oct 15, 2021

Add an auto-configuration builder. #3753

Merged

jack-berg mentioned this pull request Oct 19, 2021

AttributesBuilder.remove #3068

Closed

anuraaga closed this Oct 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a AutoConfiguredComponentWrapper SPI #3714

Add a AutoConfiguredComponentWrapper SPI #3714

anuraaga commented Oct 8, 2021 •

edited

Loading

anuraaga left a comment

anuraaga Oct 8, 2021

jsuereth Oct 8, 2021

edwardxia Oct 8, 2021

jack-berg Oct 8, 2021

edwardxia Oct 8, 2021 •

edited

Loading

anuraaga Oct 9, 2021

anuraaga Oct 11, 2021

jack-berg Oct 11, 2021

anuraaga commented Oct 8, 2021

edwardxia commented Oct 8, 2021

jkwatson Oct 11, 2021

anuraaga Oct 12, 2021

jkwatson Oct 11, 2021

edwardxia Oct 11, 2021

jkwatson commented Oct 11, 2021

anuraaga commented Oct 12, 2021

jkwatson commented Oct 12, 2021

anuraaga Oct 14, 2021

codecov bot commented Oct 14, 2021

jkwatson Oct 14, 2021

jkwatson Oct 14, 2021

anuraaga commented Oct 23, 2021

Add a AutoConfiguredComponentWrapper SPI #3714

Add a AutoConfiguredComponentWrapper SPI #3714

Conversation

anuraaga commented Oct 8, 2021 • edited Loading

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardxia Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga commented Oct 8, 2021

edwardxia commented Oct 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkwatson commented Oct 11, 2021

anuraaga commented Oct 12, 2021

jkwatson commented Oct 12, 2021

Choose a reason for hiding this comment

codecov bot commented Oct 14, 2021

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga commented Oct 23, 2021

anuraaga commented Oct 8, 2021 •

edited

Loading

edwardxia Oct 8, 2021 •

edited

Loading