Skip to content
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

#1121 - configure metric/trace otlp connections #1202

Closed
wants to merge 10 commits into from

Conversation

stefanprisca
Copy link
Contributor

@stefanprisca stefanprisca commented Sep 23, 2020

Changes for issue #1121

This allows metrics and traces to be sent to different endpoints.
Changes include:

  • Add declarative endpoint connection configuration
  • Change Exporter interface to take in different traces/metrics config
  • Move connection logic in otlpConnection object
  • Update tests and add tests for connection configuration logic

The idea of the design is to have a configurable connection, which can be used by both metrics and traces.
By moving all the connection logic into a new otlpConnection object, the Exporter can make use of this to configure different connections for traces and metrics. Moreover, by using a newConnectionHandler callback, the Exporter can create specific metric/traces clients from the corresponding otlpConnections. These clients are then used to export the metrics and traces.
For example, creating the metrics connection is done as such:

 e.metricsConnection = newOtlpConnection(e.handleNewMetricsConnection, metricsConfig) 

The handleNewMetricsConnection is the newConnectionHandler for metrics, which will create a colmetricpb.NewMetricsServiceClient grpc client for sending metrics. The second argument is the metrics specific configuration.

This flow is also described in this diagram: https://github.com/stefanprisca/opentelemetry-go/blob/otlp-exporters-1121/exporters/otlp/1121/OtlpExporterDesign-Page-1.png

Moreover, in order to handle different configurations, the same ExporterOptions are used, but the NewExporter/NewUnstartedExporter interface changes to take a new structure containig both metrics and traces configurations: NewExporter(config ConnConfigurations). In order to make it easier to construct these configurations, the ConnConfigurations structure is exported, and a new set of declarative functions introduced:

type ConnConfigurations struct {
	metrics config
	traces  config
}

// NewConnections creates default configurations for both metrics and traces and applies
// any ExporterOptions provided to both metrics and traces connection.
// Use this when configuring both metrics and traces endpoints.
// Specific options (like collector address or dial options) can be set for both signals
// with `SetMetricOptions`, `SetTraceOptions` or `SetCommonOptions`
func NewConnections(opts ...ExporterOption) ConnConfigurations

// NewTracesConnection creates default configuration for traces and applies
// any ExporterOptions provided to the traces connection.
// Use this when configuring only a traces endpoint.
// Specific options (like collector address or dial options) can be further set with `SetTraceOptions`
func NewTracesConnection(opts ...ExporterOption) ConnConfigurations

// NewMetricsConnection creates default configuration for metrics and applies
// any ExporterOptions provided to the metrics connection.
// Use this when configuring only a metrics endpoint.
// Specific options (like collector address or dial options) can be further set with `SetMetricOptions`
func NewMetricsConnection(opts ...ExporterOption) ConnConfigurations

// SetCommonOptions updates the connection configuration for both metrics and traces,
// allowing to override default or custom options for both endpoints at once.
func (occ ConnConfigurations) SetCommonOptions(opts ...ExporterOption) ConnConfigurations

// SetMetricOptions updates the connection configuration for the metrics endpoint,
// overriding options set with NewConnections, or setting new metrics specific options.
func (occ ConnConfigurations) SetMetricOptions(opts ...ExporterOption) ConnConfigurations

// SetTraceOptions updates the connection configuration for the traces endpoint,
// overriding options set with NewConnections, or setting new trace specific options.
func (occ ConnConfigurations) SetTraceOptions(opts ...ExporterOption) ConnConfigurations

This allows metrics and traces to be sent to different endpoints.
Changes include:
* Export ConnectionConfiguration structure
* Add NewConnectionConfig to easily create configurations
* Change Exporter interface to take in different traces/metrics config
* Move connection logic in otlpConnection object
* Update tests
otlp.WithAddress("localhost:30080"),
otlp.WithGRPCDialOption(grpc.WithBlock()), // useful for testing
)
exp, err := otlp.NewExporter(config, config)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NewExporter takes in two arguments: a configuration for the metrics connection and one for the traces

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this use functional options instead? That would allow a user to choose to configure only metrics or traces and could also provide a convenience option for using the same connection configuration for both. E.g.:

// only provide a ConnectionConfiguration for metrics, traces would not be exported
exp, err := otlp.NewExporter(otlp.WithMetricsConnection(config))

or

// use the same ConnectionConfiguration for both signal types
exp, err := otlp.NewExporter(otlp.WithConnection(config))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True! But the NewExporter needs to take in a structure of the form:

type ConnConfigurations struct {
	metrics config
	traces  config
}

And then you'd have more functional methods like:

func NewConnConfigurations() ConnConfigurations
func (occ ConnConfigurations) WithConnection(opts ...ExporterOption) ConnConfigurations
func (occ ConnConfigurations) WithMetricsConnection(opts ...ExporterOption) ConnConfigurations
func (occ ConnConfigurations) WithTracesConnection(opts ...ExporterOption) ConnConfigurations

Resulting in client calls like so:

exp, err := otlp.NewExporter(otlp.NewConnConfigurations().
		WithTracesConnection(otlp.WithTLSCredentials(creds)))

Is this what you are thinking?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Aneurysm9 a problem with the way you suggested is that if someone wants to configure different trace and metrics options, you'd still have a method like WithMetricsAndTracesConfigurations(config, config). Or is there something I am missing?

I update the interface a bit, to make it more declarative. I think that this works quite well.
It uses a structure holding configuration for both metrics and traces:

type ConnConfigurations struct {
	metrics config
	traces  config
}

Then adds a declarative interface on top, allowing users to configure common settings and custom settings on the specific signals:

// NewConnections initializes a config struct with default values and applies
// any ExporterOptions provided for both metrics and traces.
func NewConnections(opts ...ExporterOption) ConnConfigurations

// SetMetricOptions updates the connection configuration for the metrics endpoint,
// overriding options set with NewConnections, or setting new metrics specific options.
func (occ ConnConfigurations) SetMetricOptions(opts ...ExporterOption) ConnConfigurations

// SetTraceOptions updates the connection configuration for the traces endpoint,
// overriding options set with NewConnections, or setting new trace specific options.
func (occ ConnConfigurations) SetTraceOptions(opts ...ExporterOption) ConnConfigurations

There's some test cases for this in exporters/otlp/options_test.go showing the uses.

Looking forward to hearing your feedback!

)

func (e *Exporter) lastConnectError() error {
errPtr := (*error)(atomic.LoadPointer(&e.lastConnectErrPtr))
type otlpConnection struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All connection logic is contained in this object. The otlpConnection carries it's own configuration, and is responsible for dialing the collector, calling the newConnectionHandler callback and maintaining the connection alive.

@codecov
Copy link

codecov bot commented Sep 23, 2020

Codecov Report

Merging #1202 into master will increase coverage by 0.0%.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1202   +/-   ##
======================================
  Coverage    76.5%   76.6%           
======================================
  Files         134     134           
  Lines        5801    5860   +59     
======================================
+ Hits         4442    4492   +50     
- Misses       1110    1115    +5     
- Partials      249     253    +4     
Impacted Files Coverage Δ
exporters/otlp/connection.go 83.6% <ø> (-11.2%) ⬇️
exporters/otlp/options.go 76.7% <ø> (+26.7%) ⬆️
exporters/otlp/otlp.go 76.9% <ø> (+<0.1%) ⬆️

e.metricExporter = colmetricpb.NewMetricsServiceClient(cc)
e.mu.Unlock()

// TODO @sprisca: would a signal on the stop chanel stop both connections?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what the stopCh is being used for. I have not seen any actual write to this channel, which would stop the background connections (maybe I'm missing something?)
Either way, now that there are two connections there should be two channels also, since one connection will miss the stop signal if the other one consumes it first. What do you think?

if !e.connected() {
return nil
if !e.tracesConnection.connected() {
return errDisconnected
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason why this was returning nil instead of throwing an error?

@stefanprisca stefanprisca changed the title #1121 - configure metric/trace otlp connections [WIP] #1121 - configure metric/trace otlp connections Sep 23, 2020
@stefanprisca
Copy link
Contributor Author

I can't figure out why the precommit step has failed. The pointer offset alignment test is passing on my computer, but I don't understand why it's failing on the test machine, and I don't have access to debug the build itself. Any idea why this test would be failing with the following error?

struct fields not aligned for 64-bit atomic operations:
  otlpConnection.lastConnectErrPtr: 92-byte offset

Any help is appreciated!

@MrAlias
Copy link
Contributor

MrAlias commented Sep 24, 2020

I can't figure out why the precommit step has failed. The pointer offset alignment test is passing on my computer, but I don't understand why it's failing on the test machine, and I don't have access to debug the build itself. Any idea why this test would be failing with the following error?

struct fields not aligned for 64-bit atomic operations:
  otlpConnection.lastConnectErrPtr: 92-byte offset

Any help is appreciated!

I haven't looked too deep into the changes yet, but to ensure 64-bit alignment of a struct field make sure that is the first field declared in the struct.

@evantorrie
Copy link
Contributor

struct fields not aligned for 64-bit atomic operations:
  otlpConnection.lastConnectErrPtr: 92-byte offset

@stefanprisca Thanks for the work on this PR!

The CircleCI tests run in both 64-bit x86_64 and 32-bit i386 modes. On i386, various structures (particularly pointers to memory) are 32-bits long rather than 64-bits long, and this may result in subsequent fields in a struct not being 8-byte aligned. Most desktop machines these days are 64-bit, so you probably won't see this running on your machine at home.

We recommend putting any values that need to have atomic operations performed on them at the beginning of the struct, since structs are guaranteed to be 64-bit aligned even on a 32-bit machine.

For more than you probably ever wanted to know about Go memory alignment, you may want to read this page.

@stefanprisca
Copy link
Contributor Author

stefanprisca commented Sep 25, 2020

The CircleCI tests run in both 64-bit x86_64 and 32-bit i386 modes. On i386, various structures (particularly pointers to memory) are 32-bits long rather than 64-bits long, and this may result in subsequent fields in a struct not being 8-byte aligned.

@evantorrie Thanks for the clarification! I suspected that this would be the issue, but had no idea how golang treats memory. The article is really useful!

Just out of curiosity, it seems a bit strange that the previous Exporter type also didn't have the unsafe pointer as the first field. So why was that 8Byte aligned, and this isn't?

* update error pointer field so it's 64bit alligned
* change to declarative configurations interface
* Set common options at configuration creation
* Make SetMetrics/TracesOptions override the common options
* add tests for setting common options, and overriding specific metrics/traces options
@stefanprisca stefanprisca changed the title [WIP] #1121 - configure metric/trace otlp connections #1121 - configure metric/trace otlp connections Sep 25, 2020
@jmacd
Copy link
Contributor

jmacd commented Sep 29, 2020

While I don't see a problem with this change, I also don't have a problem with the way it is now. The Lightstep OTel launcher creates two OTLP exporters, each with their own endpoint configured, then uses the two independent exporters to setup the metrics and tracing SDKs. There is no connection between the two exporters with their different signals and endpoints. What advantages do I get if a single exporter manages both?

Example: https://github.com/lightstep/otel-launcher-go/blob/27573ba6e8397f17315665df3ec615bbe7036cee/launcher/config.go#L321

@MrAlias
Copy link
Contributor

MrAlias commented Sep 29, 2020

While I don't see a problem with this change, I also don't have a problem with the way it is now. The Lightstep OTel launcher creates two OTLP exporters, each with their own endpoint configured, then uses the two independent exporters to setup the metrics and tracing SDKs. There is no connection between the two exporters with their different signals and endpoints. What advantages do I get if a single exporter manages both?

That's a fair question. The issue this was trying to solve is to have us conform to the specification which requires that the exporter be configurable to send different signals to different places.

@stefanprisca
Copy link
Contributor Author

@jmacd that's true. But what does this mean for this PR? Should we continue with it, or reconsider? The changes are ready for reviewing if we decide to continue with this design.

If specific signal connections are not set, then do not try starting a connection with default settings.
Connections should not be started for non-existant collectors.

Changes:
* Exporter condition for starting connection if it has address set
* Configuration options change to not set default options
* Export DefaultConnectionOptions allowing users to set them
* Update tests

// SetCommonOptions updates the connection configuration for both metrics and traces,
// allowing to override default or custom options for both endpoints at once.
func (occ ConnConfigurations) SetCommonOptions(opts ...ExporterOption) ConnConfigurations {

This comment was marked as outdated.

e.metadata = metadata.New(e.c.headers)

// Only create connections for the signals with configured addresses.
if configuration.metrics.collectorAddr != "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A connection for a signal is only created if an address has been given for that signal. Otherwise it's considered the client does not want to use that signal, so no connection is needed.

@@ -91,3 +97,88 @@ func TestExporterShutdownNoError(t *testing.T) {
t.Errorf("shutdown errored: expected nil, got %v", err)
}
}

func TestExporterOnlyStartsMetricsConnection(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added new tests to ensure that connections are handled as expected.

The previous design required clients to always pass in DefaultOptions.
As we don't want that (probably), this adds the defaults automatically.
Specific metrics and trace connections are created by separate methods.

* make DefaultOptions internal again
* Add NewMetricsConnection
* Add NewTracesConnection
* New*Connection methods will apply default options automatically
* Update tests
@stefanprisca
Copy link
Contributor Author

Changed the options interface again (see PR description)....
There was an issue with setting default connections for both metrics and traces. If the client wants only traces, and sets a custom address for the collector, then the metrics connection would start also and keep retrying to connect at the default address.
I hope to solve it by adding specific trace/metrics configuration constructors, but it's still not waterproof as it highly depends on what the client will configure and it can be messed up. The other option (my initial thought) was to have clients explicitly set DefaultOptions, but this results in the DefaultOptions being specifically set every time a new exporter is initialized which can be bothersome. On the other hand it might be better to be as specific as possible, and considering that exporters are only initialized once it might not be that troublesome.

@MrAlias I'll be working on some examples, but I think that this change needs more discussion as the interface is a bit tricky to get right. Considering this, if you feel like it would be better to work on it from the same timezone, please take it over :)

@stefanprisca
Copy link
Contributor Author

Hi, unfortunately I cannot contribute to this change any longer as I'm starting a new job next week and my individual CLA becomes invalid (and will be taking a break from open source for a while).
I'm looking forward to seeing version 1.0 of OpenTelemetry out there, and hopefully contributing again at some point!

@Aneurysm9
Copy link
Member

Hi, unfortunately I cannot contribute to this change any longer as I'm starting a new job next week and my individual CLA becomes invalid (and will be taking a break from open source for a while).
I'm looking forward to seeing version 1.0 of OpenTelemetry out there, and hopefully contributing again at some point!

Thank you for your efforts and good luck with the new job! We appreciate the work you did on this and I'm sure we can find someone to push it over the finish line.

@krnowak
Copy link
Member

krnowak commented Nov 11, 2020

I'd like to take it over to push #1085 forward.

@MrAlias
Copy link
Contributor

MrAlias commented Nov 11, 2020

@krnowak sounds good 👍 . I'll leave it up to you if you want to close this PR and open another or build on to this.

@seanschade
Copy link
Contributor

I'd like to take it over to push #1085 forward.

@krnowak is the plan to close this and start over? As a consumer it would be nice to see a more idiomatic solution. I would be happy to lend a hand as well. I had to implement this recently myself. We use https://github.com/sethvargo/go-envconfig so you can ignore the struct tags, but it allows me to initialize our exporter with a one-liner.

exporter := otlp.NewUnstartedExporter(cfg.Exporter.OTLP.Options()...)

Here's what my configuration struct looks like.

package telemetry

import (
	"crypto/tls"
	"strings"
	"time"

	"go.opentelemetry.io/otel/exporters/otlp"
	"google.golang.org/grpc/credentials"
)

// OpenTelemetry Protocol Exporter.
//
// The following configuration options MUST be available to configure the OTLP exporter.
// Each configuration option MUST be overridable by a signal specific option.
//
// See: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/protocol/exporter.md#opentelemetry-protocol-exporter
type OTLP struct {
	// Target to which the exporter is going to send spans or metrics.
	Endpoint string `env:"OTEL_EXPORTER_OTLP_ENDPOINT,default=localhost:55680"`

	// The protocol used to transmit the data.One of grpc,http/json,http/protobuf.
	//
	// Note: Note currently supported by v0.13.0
	Protocol string `env:"OTEL_EXPORTER_OTLP_PROTOCOL,default=grpc"`

	// Whether to enable client transport security for the exporter's grpc or http connection.
	Insecure bool `env:"OTEL_EXPORTER_OTLP_INSECURE,default=false"`

	// Compression key for supported compression types. Supported compression: gzip
	Compression string `env:"OTEL_EXPORTER_OTLP_COMPRESSION"`

	// Key-value pairs to be used as headers associated with gRPC or HTTP requests.
	// See: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/protocol/exporter.md#specifying-headers-via-environment-variables
	Headers string `env:"OTEL_EXPORTER_OTLP_HEADERS"`

	// Max waiting time for the backend to process each spans or metrics batch.
	//
	// Note: Not currently supported by v0.13.0
	Timeout time.Duration `env:"OTEL_EXPORTER_OTLP_TIMEOUT,default=10s"`
}

func (e *OTLP) WithEndpoint() otlp.ExporterOption {
	return otlp.WithAddress(e.Endpoint)
}

func (e *OTLP) WithTransportSecurity() otlp.ExporterOption {
	if e.Insecure {
		return otlp.WithInsecure()
	}
	return otlp.WithTLSCredentials(credentials.NewTLS(&tls.Config{MinVersion: tls.VersionTLS12}))
}

func (e *OTLP) WithCompressor() otlp.ExporterOption {
	return otlp.WithCompressor(e.Compression)
}

func (e *OTLP) WithHeaders() otlp.ExporterOption {
	return otlp.WithHeaders(StringKVToMap(e.Headers))
}

func (e *OTLP) Options() []otlp.ExporterOption {
	return []otlp.ExporterOption{
		e.WithEndpoint(),
		e.WithTransportSecurity(),
		e.WithCompressor(),
		e.WithHeaders(),
	}
}

// StringKVToMap parses a string according to the W3C Baggage format.
// See: https://github.com/w3c/baggage/blob/master/baggage/HTTP_HEADER_FORMAT.md
func StringKVToMap(kv string) map[string]string {
	m := map[string]string{}
	if attrs := strings.TrimSpace(kv); len(attrs) > 0 {
		for _, p := range strings.Split(attrs, ",") {
			field := strings.SplitN(p, "=", 2)
			if len(field) != 2 {
				// Ignore invalid values.
				continue
			}

			k, v := strings.TrimSpace(field[0]), strings.TrimSpace(field[1])
			if len(k) > 0 && len(v) > 0 {
				m[k] = v
			}
		}
	}
	return m
}

@krnowak
Copy link
Member

krnowak commented Nov 23, 2020

@seanschade: My work so far is here: https://github.com/krnowak/opentelemetry-go/tree/otlp-conn (see exporters/otlp/grpc directory), it's still WIP, but I'm slowly upstreaming the parts of the branch.

What I have is more or less (writing from memory, so names may not be exactly same):

type GRPCConnectionConfig struct {
	// unexported fields here
}

type ConnectionOption func(*GRPCConnectionConfig)

func (c GRPCConnectionConfig) Apply(ConnectionOption ...opts) GRPCConnectionConfig { … }
func (c *GRPCConnectionConfig) ApplyInPlace(ConnectionOption ...opts) { … }

func WithCompressor(…) ConnectionOption { … }
// more options

// there will be more implementation of this interface, currently there is only for GRPC
type ConnectionManager interface { … }

func NewGRPCSingleConnectionManager(cfg GRPCConnectionConfig) ConnectionManager { … }

type MultiConnectionConfig struct {
	Tracing GRPCConnectionConfig
	Metrics GRPCConnectionConfig
}

func NewGRPCMultiConnectionManager(cfg MultiConnectionConfig) ConnectionManager { … }

type config struct {
	// unexported fields here
}

type ExporterOption func(*config)

func NewExporter(cm ConnectionManager, ExporterOption ...opts) Exporter { … }

so the API above would be used like:

cfg := GRPCConnectionConfig{}.Apply(WithCompressor(…), WithFoo(…))
cm := NewGRPCSingleConnectionConfig(cfg)
exporter := NewExporter(cm)

// or

cfg := GRPCMultiConnectionConfig{}
cfg.Tracing.ApplyInPlace(WithFoo(…), WithBar(…))
cfg.Metrics.ApplyInPlace(WithFoo(…), WithStuff(…))
cm := NewGRPCMultiConnectionManager(cfg)
exporter := NewExporter(cm)

This is what I have, maybe it will change. This is something to figure out as a part of #536.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants