Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[feature] Design doc for Go Launcher #2592
[feature] Design doc for Go Launcher #2592
Changes from 2 commits
0285570
4fd47fc
e96e2cc
e039649
6c42787
7355533
7501745
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For expoter-options below, do these assume OTLP is the exporter, or would they be expected to work with "registered" exporters as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The launcher uses the
BatchSpanProcessor
by default. There currently is not a plan to swap out the batch processor with, for example, theSimpleSpanProcessor
. 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I certainly wouldn't suggest using the
SimpleSpanProcessor
, but theBatchSpanProcessor
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When issues are created from this, we need to ensure this can also be set via
OTEL_RESOURCE_ATTRIBUTES
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the goal is to support all
OTEL_*
environment variables and as such, honor a service name set inOTEL_RESOURCE_ATTRIBUTES
. Agreed that it shouldn't really be required per se, as it should have the default ofunknown_service:<process>
. I will update this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've clarified the service name requirement in e039649
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
OTEL_TRACES_EXPORTER
andOTEL_METRICS_EXPORTER
(and eventuallyOTEL_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 toautoprop
needs to be created as an item for this project.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal would be to support all
OTEL_*
environment variables. The interface for setting configuration options in code, likeWithTracesExporterEndpoint
, would probably remain more limited.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These options seem generic across all exports yet they pair with OTLP exporter environment variables. How do we plan to support other exporter configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be important to get right, otherwise the promise of vendor neutrality is hollow and only applies to vendors that accept OTLP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this will be extensible. Possibly by using the
autoprop
package to allow users to register their own propagators.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're understanding this right, the
autoprop
package seems like the right move for specifying propagators, and later, theautoexporter
(?) package will be useful for specifying exporters. Sinceautoprop
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work? Does the
launcher
package export some API that can be used by thehoneycomb
package ininit()
, thus allowing the import for side effects pattern? What is that API?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super fond of silently imported packages. How about something like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this would return a
launcher.Option
, probably by wrappinglauncher.WithHeaders()
like this:Would the
launcher.Option
type operate on an exportedlauncher.Config
type allowing the implementation of wholly-new option implementations, or would everything need to be done in terms of existinglauncher.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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
This adds slightly more boilerplate, but also makes things somewhat more consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.