opentelemetry-go-contrib icon indicating copy to clipboard operation
opentelemetry-go-contrib copied to clipboard

[feature] Design doc for Go Launcher

Open JamieDanielson opened this issue 3 years ago • 6 comments
trafficstars

During today's SIG meeting, we discussed moving the design doc linked on #2591 into a PR for a central location and ease of leaving feedback on specific sections of the doc.

A branch (WIP) for further review of the code can be found in the Honeycomb fork of this repo.

JamieDanielson avatar Jul 21 '22 17:07 JamieDanielson

From the SIG meeting: The next steps for this are:

  • Ratify this design by getting enough approvals to merge it
  • Building a project to track all work needed.
  • Build the project on a separate feature branch (not included in the SIG meeting, but should be done)
  • Merge the completed project into the main branch and release.

MrAlias avatar Jul 22 '22 14:07 MrAlias

Codecov Report

Merging #2592 (7501745) into main (e2cab2e) will decrease coverage by 4.9%. The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2592     +/-   ##
=======================================
- Coverage   74.3%   69.4%   -5.0%     
=======================================
  Files        144     145      +1     
  Lines       6569    6707    +138     
=======================================
- Hits        4883    4655    -228     
- Misses      1543    1938    +395     
+ Partials     143     114     -29     
Impacted Files Coverage Δ
instrumentation/host/version.go 0.0% <0.0%> (-100.0%) :arrow_down:
instrumentation/runtime/version.go 0.0% <0.0%> (-100.0%) :arrow_down:
instrumentation/host/host.go 0.0% <0.0%> (-74.5%) :arrow_down:
instrumentation/runtime/runtime.go 0.0% <0.0%> (-74.1%) :arrow_down:
...ation/google.golang.org/grpc/otelgrpc/grpctrace.go 49.1% <0.0%> (-8.4%) :arrow_down:
instrumentation/net/http/otelhttp/config.go 80.3% <0.0%> (-7.6%) :arrow_down:
...ion/google.golang.org/grpc/otelgrpc/interceptor.go 84.2% <0.0%> (-2.4%) :arrow_down:
...n/github.com/Shopify/sarama/otelsarama/producer.go 93.7% <0.0%> (-1.0%) :arrow_down:
detectors/aws/ecs/ecs.go 54.7% <0.0%> (ø)
propagators/b3/b3_config.go 100.0% <0.0%> (ø)
... and 9 more

codecov[bot] avatar Jul 26 '22 17:07 codecov[bot]

Thanks for all the feedback here. Please let me know if there are any other items to address for the initial design document, or anything else I can do to help clarify any questions.

JamieDanielson avatar Aug 02 '22 18:08 JamieDanielson

I'm still not thrilled with the idea of a new code config surface when we already have one. I would prefer a solution which:

  • Doesn't introduce a new set of options for either the core library or for vendors (as GCP, I don't want to maintain 2 sets of options either).
  • Can set up OpenTelemetry with a few lines go code.
  • Can support "auto" vendor exporters and detector usage

I'll propose an alternative for consideration here:

The launcher

Introduce a new package, launcher, which each has two functions:

func NewTracerProvider(opts ...sdktrace.TracerProviderOption) *TracerProvider { ...

By default, it uses an OTLP exporter, and keeps all other defaults. This should make it possible to do:

otel.SetTracerProvider(launcher.NewTracerProvider())
defer otel.GetTracerProvider().Shutdown()

It is trivial for users to switch between this and the sdktrace TracerProvider. However, it isn't easy for vendors to use the launcher (yet).

Auto exporter config

Introduce a package which (TBD) allows configuration of ~any exporter without code changes. It defaults to using the OTLP exporter. Update the simple launcher above to use this exporter by default, which does not change default behavior of the simple launcher.

Auto resource config

Introduce package which (TBD) allows configuration of ~any resource detector without code changes. It defaults to using the default set of OTel detectors. Update the simple launcher above to use this resource by default, which does not change default behavior of the simple launcher.

dashpole avatar Aug 04 '22 19:08 dashpole

Bikeshedding alert:

What if instead of launcher it's called otelinit?

To me, launcher doesn't mean a whole lot. But otelinit does: initialize opentelemetry.

cartermp avatar Aug 18 '22 18:08 cartermp

Bikeshedding alert:

What if instead of launcher it's called otelinit?

To me, launcher doesn't mean a whole lot. But otelinit does: initialize opentelemetry.

I've renamed Launcher to Initialization Layer, otelinit in code, so we can see how that looks. Any objections?

JamieDanielson avatar Aug 29 '22 20:08 JamieDanielson

This design has now been updated to include the autoexport package (currently in another PR) and the autoprop package. The intent is to use the configuration options available with those instead of re-creating them here, so the OTLP-specific exporter configuration options have been dropped from this as well.

This design should now allow for setting defaults (e.g. otlp/grpc/tracecontext/baggage) while still allowing for the ability to override - some override code configuration examples are provided for exporter and propagator. This should help keep the promise of minimal code necessary to initialize, without adding too much extra code surface to maintain, and avoiding the huge leap to manually configure various options for a more customized setup.

Please review the updated document and let me know if there are open questions that have not been addressed here that require changes to this doc. Thanks!

JamieDanielson avatar Sep 30 '22 18:09 JamieDanielson

@open-telemetry/go-approvers @open-telemetry/proto-go-maintainers looking for feedback on this design doc now we've updated to use autoexport 👍🏻

MikeGoldsmith avatar Oct 25 '22 10:10 MikeGoldsmith

Note: The "launcher/init" code has been moved into its own repo in the Honeycombio org, honeycombio/otel-launcher-go. Please direct any feedback there as we continue to iterate on it. As discussed with the SIG, we will revisit the contribution in a few months.

JamieDanielson avatar Jan 18 '23 18:01 JamieDanielson