[feature] Design doc for Go Launcher #2592

JamieDanielson · 2022-07-21T17:48:10Z

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.

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.

MrAlias · 2022-07-22T14:39:21Z

design/0000-launcher.md

+
+| Config Option               | Env Variable                        | Required | Default              |
+| --------------------------  | ----------------------------------- | -------- | -------------------- |
+| WithServiceName             | OTEL_SERVICE_NAME                   | y        | -                    |


When issues are created from this, we need to ensure this can also be set via OTEL_RESOURCE_ATTRIBUTES

Can we just make it not required? Won't the SDK pull from the environment if a resource is provided that doesn't have a service name?

Yes, the goal is to support all OTEL_* environment variables and as such, honor a service name set in OTEL_RESOURCE_ATTRIBUTES. Agreed that it shouldn't really be required per se, as it should have the default of unknown_service:<process>. I will update this.

I've clarified the service name requirement in e039649

MrAlias · 2022-07-22T14:42:21Z

design/0000-launcher.md

+| WithMetricsExporterEndpoint | OTEL_EXPORTER_OTLP_METRICS_ENDPOINT | n        | localhost:4317       |
+| WithMetricsExporterInsecure | OTEL_EXPORTER_OTLP_METRICS_INSECURE | n        | false                |
+| WithLogLevel                | OTEL_LOG_LEVEL                      | n        | info                 |
+| WithPropagators             | OTEL_PROPAGATORS                    | n        | tracecontext,baggage |


Ideally this will be extensible. Possibly by using the autoprop package to allow users to register their own propagators.

Seconded. It should use the autoprop.GetTextMapPropagator() function being added Soon™. When a similar registry is created for exporters it should also have this ability.

If we're understanding this right, the autoprop package seems like the right move for specifying propagators, and later, the autoexporter(?) package will be useful for specifying exporters. Since autoprop is already merged in we'll be able to use that sooner rather than later, and for now keep the exporters limited to http and grpc as planned.

Design has been updated to remove OTLP-specific configuration options as they would be handled with the exporter itself; the design now specifies using the autoexport package.

MrAlias · 2022-07-22T14:46:19Z

design/0000-launcher.md

+| WithServiceName             | OTEL_SERVICE_NAME                   | y        | -                    |
+| WithServiceVersion          | OTEL_SERVICE_VERSION                | n        | -                    |
+| WithHeaders                 | OTEL_EXPORTER_OTLP_HEADERS          | n        | {}                   |
+| WithTracesExporterEndpoint  | OTEL_EXPORTER_OTLP_TRACES_ENDPOINT  | n        | localhost:4317       |


The OTEL_TRACES_EXPORTER and OTEL_METRICS_EXPORTER (and eventually OTEL_LOGS_EXPORTER) environment variables should be supported. Ideally, in the same way the propagators are: by allowing users to register their own and then call them here. This likely means an "autoexport" similar to autoprop needs to be created as an item for this project.

The goal would be to support all OTEL_* environment variables. The interface for setting configuration options in code, like WithTracesExporterEndpoint, would probably remain more limited.

Design has been updated to remove OTLP-specific configuration options as they would be handled with the exporter itself; the design now specifies using the autoexport package.

MrAlias · 2022-07-22T14:51:13Z

design/0000-launcher.md

+| WithHeaders                 | OTEL_EXPORTER_OTLP_HEADERS          | n        | {}                   |
+| WithTracesExporterEndpoint  | OTEL_EXPORTER_OTLP_TRACES_ENDPOINT  | n        | localhost:4317       |
+| WithTracesExporterInsecure  | OTEL_EXPORTER_OTLP_TRACES_INSECURE  | n        | false                |
+| WithMetricsExporterEndpoint | OTEL_EXPORTER_OTLP_METRICS_ENDPOINT | n        | localhost:4317       |
+| WithMetricsExporterInsecure | OTEL_EXPORTER_OTLP_METRICS_INSECURE | n        | false                |


These options seem generic across all exports yet they pair with OTLP exporter environment variables. How do we plan to support other exporter configuration?

This will be important to get right, otherwise the promise of vendor neutrality is hollow and only applies to vendors that accept OTLP.

The initial intent was to have this specific to OTEL and OTLP, not to have it extend to other exporters, as OTLP is vendor-neutral but yes, would require accepting the OTLP format. Other exporter APIs could be added later on though. As a stop-gap, the collector can be used to translate to other formats.

Design has been updated to remove OTLP-specific configuration options as they would be handled with the exporter itself; the design now specifies using the autoexport package.

MrAlias · 2022-07-22T14:56:47Z

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.

design/0000-launcher.md

Aneurysm9 · 2022-07-22T21:51:44Z

design/0000-launcher.md

+
+| Config Option               | Env Variable                        | Required | Default              |
+| --------------------------  | ----------------------------------- | -------- | -------------------- |
+| WithServiceName             | OTEL_SERVICE_NAME                   | y        | -                    |


Can we just make it not required? Won't the SDK pull from the environment if a resource is provided that doesn't have a service name?

Aneurysm9 · 2022-07-22T21:55:43Z

design/0000-launcher.md

+| WithMetricsExporterEndpoint | OTEL_EXPORTER_OTLP_METRICS_ENDPOINT | n        | localhost:4317       |
+| WithMetricsExporterInsecure | OTEL_EXPORTER_OTLP_METRICS_INSECURE | n        | false                |
+| WithLogLevel                | OTEL_LOG_LEVEL                      | n        | info                 |
+| WithPropagators             | OTEL_PROPAGATORS                    | n        | tracecontext,baggage |


Seconded. It should use the autoprop.GetTextMapPropagator() function being added Soon™. When a similar registry is created for exporters it should also have this ability.

Aneurysm9 · 2022-07-22T22:02:57Z

design/0000-launcher.md

+| WithHeaders                 | OTEL_EXPORTER_OTLP_HEADERS          | n        | {}                   |
+| WithTracesExporterEndpoint  | OTEL_EXPORTER_OTLP_TRACES_ENDPOINT  | n        | localhost:4317       |
+| WithTracesExporterInsecure  | OTEL_EXPORTER_OTLP_TRACES_INSECURE  | n        | false                |
+| WithMetricsExporterEndpoint | OTEL_EXPORTER_OTLP_METRICS_ENDPOINT | n        | localhost:4317       |
+| WithMetricsExporterInsecure | OTEL_EXPORTER_OTLP_METRICS_INSECURE | n        | false                |


This will be important to get right, otherwise the promise of vendor neutrality is hollow and only applies to vendors that accept OTLP.

Aneurysm9 · 2022-07-22T22:07:04Z

design/0000-launcher.md

+```go
+import (
+    "github.com/opentelemetry-go-contrib/otel-launcher-go/launcher"
+    _ "github.com/honeycombio/otel-launcher-go/launcher/honeycomb"


How does this work? Does the launcher package export some API that can be used by the honeycomb package in init(), thus allowing the import for side effects pattern? What is that API?

This is still being worked on separately, but a peek at what the vendor code would look like lives here: https://github.com/honeycombio/otel-launcher-go/blob/mike/reorganise/honeycomb/honeycomb.go

As a side note (and what you'll see in the directory for the link above), we're planning to include a baggage span processor and dynamic attribute processor in our package, along with other niceties, though they could probably be pushed upstream if there was an appetite for it. The baggage span processor allows for easily attaching baggage to child spans, and the dynamic attribute span processor allows for adding dynamic fields to spans (like current memory util).

Once we have the vendor-specific package in a cleaner place for sharing, I can link that up.

Aneurysm9 · 2022-07-22T22:13:04Z

design/0000-launcher.md

+
+func main() {
+    lnchr, err := launcher.ConfigureOpentelemetry(
+        honeycomb.WithApiKey("api-key"),


I assume this would return a launcher.Option, probably by wrapping launcher.WithHeaders() like this:

func WithAPIKey(key string) launcher.Option { return launcher.WithHeaders(map[string]string{"HONEYCOMB_API_KEY": key}) }

Would the launcher.Option type operate on an exported launcher.Config type allowing the implementation of wholly-new option implementations, or would everything need to be done in terms of existing launcher.Option implementations? We've traditionally not exported the config type to avoid unintended/unsupportable behavior, but this might be a place where doing it make sense.

This assumption is correct, that it would return a launcher.Option.

Regarding unintended/unsupportable behavior, vendors can add config validation for their own use cases. For example, our vendor package (WIP) has a launcher.ValidateConfig that returns errors for invalid headers.

Aneurysm9 · 2022-07-22T22:19:05Z

design/0000-launcher.md

+
+## Trade-offs and mitigations
+
+This launcher is intentionally providing default configuration options, which may not precisely map out to the final configuration an end user desires. As such, there may be dependencies in the package that are not being used. For example, the default configuration will bring in both gRPC and HTTP exporters; using gRPC will result in HTTP being included but not used; similarly, using HTTP/protobuf will result in the gRPC dependency being pulled in but unused.


Could we decouple the launcher framework from the included components and then provide stock distributions that the user could select from in the same way they'd select a vendor distribution?

import ( go.opentelemetry.io/contrib/launcher _ go.opentelemetry.io/contrib/launcher/otlp/grpc // or just /otlp for both HTTP and gRPC )

This adds slightly more boilerplate, but also makes things somewhat more consistent.

We would still want to use the package that exposes both http and grpc launchers. As far as requiring the user to add their own imports here, we feel like this takes away from the point of the launcher. If someone feels really strongly about this they can use the SDK without the launcher.

Aneurysm9 · 2022-07-22T22:22:41Z

design/0000-launcher.md

+
+### Configuration Options
+
+| Config Option               | Env Variable                        | Required | Default              |


I don't see any options related to the BatchSpanProcessor. I'd assume that would be used and the user might need a way to modify it.

This makes me wonder more generally what the transition path would be for a user who outgrows the launcher and wants to convert to more explicit initialization. It would be awesome (though maybe overkill) if there was a lnchr.Generate() method that could emit the code that would be necessary to explicitly instantiate the launcher.

The launcher uses the BatchSpanProcessor by default. There currently is not a plan to swap out the batch processor with, for example, the SimpleSpanProcessor. In our other distros (Java, .NET) we have been using the batch processor with no option to swap it out, and have not yet received any feedback requesting to make it configurable.

The Generate() function you mention does sound like a great stretch goal, but it may be too much for the initial implemenetation here.

I certainly wouldn't suggest using the SimpleSpanProcessor, but the BatchSpanProcessor does have several configuration options. We should consider where the breakpoint should be that forces users to drop the simple setup path and use fully manual SDK configuration. I think that adjusting batch size or timing should probably be on the simple side of that line.

codecov · 2022-07-26T17:32:39Z

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

@@           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%)`	⬇️
instrumentation/runtime/version.go	`0.0% <0.0%> (-100.0%)`	⬇️
instrumentation/host/host.go	`0.0% <0.0%> (-74.5%)`	⬇️
instrumentation/runtime/runtime.go	`0.0% <0.0%> (-74.1%)`	⬇️
...ation/google.golang.org/grpc/otelgrpc/grpctrace.go	`49.1% <0.0%> (-8.4%)`	⬇️
instrumentation/net/http/otelhttp/config.go	`80.3% <0.0%> (-7.6%)`	⬇️
...ion/google.golang.org/grpc/otelgrpc/interceptor.go	`84.2% <0.0%> (-2.4%)`	⬇️
...n/github.com/Shopify/sarama/otelsarama/producer.go	`93.7% <0.0%> (-1.0%)`	⬇️
detectors/aws/ecs/ecs.go	`54.7% <0.0%> (ø)`
propagators/b3/b3_config.go	`100.0% <0.0%> (ø)`
... and 9 more

JamieDanielson · 2022-08-02T18:26:34Z

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.

dashpole · 2022-08-04T19:50:54Z

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.

cartermp · 2022-08-18T18:44:48Z

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.

dmathieu · 2022-08-24T18:02:05Z

design/0000-launcher.md

+)
+
+func main() {
+    lnchr, err := launcher.ConfigureOpentelemetry()


I'm not super fond of silently imported packages. How about something like this:

lnchr, err := launcher.ConfigureOpentelemetry( honey.Configure(), )

Yeah, we've had some feedback to suggest the same. We liked the idea behind it when reviewing other sources, eg sql drivers, but it's easy to miss and not see why it's not working as expected.

Both could be supported?

It seems to me that an anonymous import could easily be missed as well.
To help with that, the launcher could provide errors if something appears to be wrong, such as no vendor/provider being setup.

Both are supported today - you can use our layer without the anonymous import and add a line of code similar to what you have. We (honeycomb) may consider not enabling the anonymous import, or at least making it not the documented/preferred way, since it can be easily missed. Good feedback that you feel the same way.

Stepping back a bit, how prescriptive do you think we (opentelemetry contributors) should be here? Should we define a set of must-have/must-not-haves for vendor layers to follow?

As this feature is meant to help folks get started, I think things should be as simple and clear as possible, and avoid providing several ways to do the same thing (which goes in opposition with my previous comment, I know).
It also feels to me that we should try to let users know about anything that we can detect which seems wrong, such as telemetry configured with no provider, or a missing service name for example.
Anything that may be confusing to users when the get started, and that we can automatically detect could be provided as an information to them.

JamieDanielson · 2022-08-29T20:53:05Z

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?

dashpole · 2022-09-01T21:12:46Z

design/0000-launcher.md

+func main() {
+    init, err := otelinit.ConfigureOpentelemetry(
+        otelinit.WithServiceName("service-name"),
+        otelinit.WithHeaders(map[string]string{


Is this implying that there is a common set of configuration for all exporters? What happens if I set WithHeaders and also specify the stdout exporter?

Design has been updated to remove OTLP-specific configuration options as they would be handled with the exporter itself; the design now specifies using the autoexport package.

dashpole · 2022-09-01T21:46:14Z

design/0000-launcher.md

+}
+```
+
+### Configuration Options


For expoter-options below, do these assume OTLP is the exporter, or would they be expected to work with "registered" exporters as well?

Design has been updated to remove OTLP-specific configuration options as they would be handled with the exporter itself; the design now specifies using the autoexport package.

JamieDanielson · 2022-09-30T18:11:12Z

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!

MikeGoldsmith · 2022-10-25T10:39:00Z

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

seankhliao · 2022-12-01T12:10:01Z

design/0000-launcher.md

+func main() {
+    init, err := otelinit.ConfigureOpentelemetry(
+        otelinit.WithTracesExporter(new trace.SpanExporter("jaeger")) // if using non-default exporter
+        otelinit.WithPropagators(b3.New()) // if using non-default propagator


do the imports for trace and b3 come from api / sdk or something under otelinit?

JamieDanielson · 2023-01-18T18:16:25Z

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.

create launcher design doc

0285570

JamieDanielson marked this pull request as ready for review July 21, 2022 17:49

JamieDanielson requested review from jmacd, MrAlias, Aneurysm9, evantorrie, XSAM, dashpole, MadVikingGod, pellared, hanyuancheung and dmathieu as code owners July 21, 2022 17:49

JamieDanielson mentioned this pull request Jul 21, 2022

OpenTelemetry Go Launcher #2591

Open

clarify example showing honeycomb api key

4fd47fc

Aneurysm9 added the Skip Changelog Allow PR to succeed without requiring an addition to the CHANGELOG label Jul 21, 2022

MrAlias reviewed Jul 22, 2022

View reviewed changes

Aneurysm9 reviewed Jul 22, 2022

View reviewed changes

deejgregor mentioned this pull request Jul 24, 2022

autoconfigure #2320

Open

replace import with vanity url

e96e2cc

jmacd mentioned this pull request Jul 29, 2022

Donation proposal: OpenTelemetry Go Automatic Instrumentation open-telemetry/opentelemetry-go-instrumentation#2

Closed

clarify service name defaults

e039649

clarify what it isnt, update config options

6c42787

dmathieu reviewed Aug 24, 2022

View reviewed changes

Rename Launcher to Initialization Layer

7355533

dashpole reviewed Sep 1, 2022

View reviewed changes

MikeGoldsmith mentioned this pull request Sep 15, 2022

Add autoexport to automatically configure trace exporters #2753

Merged

Update to use autoexport package

7501745

JamieDanielson requested review from MrAlias and Aneurysm9 and removed request for MrAlias September 30, 2022 18:11

seankhliao reviewed Dec 1, 2022

View reviewed changes

JamieDanielson closed this Jan 18, 2023


		## Trade-offs and mitigations

		This launcher is intentionally providing default configuration options, which may not precisely map out to the final configuration an end user desires. As such, there may be dependencies in the package that are not being used. For example, the default configuration will bring in both gRPC and HTTP exporters; using gRPC will result in HTTP being included but not used; similarly, using HTTP/protobuf will result in the gRPC dependency being pulled in but unused.


		### Configuration Options

		\| Config Option \| Env Variable \| Required \| Default \|

[feature] Design doc for Go Launcher #2592

[feature] Design doc for Go Launcher #2592

Conversation

JamieDanielson commented Jul 21, 2022 • edited

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

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

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

MrAlias commented Jul 22, 2022

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 26, 2022 • edited

Codecov Report

JamieDanielson commented Aug 2, 2022

dashpole commented Aug 4, 2022

The launcher

Auto exporter config

Auto resource config

cartermp commented Aug 18, 2022

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

JamieDanielson commented Aug 29, 2022

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

JamieDanielson commented Sep 30, 2022

MikeGoldsmith commented Oct 25, 2022 • edited

Choose a reason for hiding this comment

JamieDanielson commented Jan 18, 2023

JamieDanielson commented Jul 21, 2022 •

edited

codecov bot commented Jul 26, 2022 •

edited

MikeGoldsmith commented Oct 25, 2022 •

edited