[client, consumer] Move client features to consumerconnection #9996
Conversation
TylerHelmuth
force-pushed
the
client-move-to-consumer
branch
from
e3d2839
to
bc7f2f5
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9996 +/- ##
==========================================
+ Coverage 91.77% 91.78% +0.01%
==========================================
Files 359 361 +2
Lines 16633 16657 +24
==========================================
+ Hits 15265 15289 +24
Misses 1041 1041
Partials 327 327 ☔ View full report in Codecov by Sentry. |
| // receivers: [otlp] | ||
| // processors: [authprinter] | ||
| // exporters: [debug] | ||
| package consumerconnection // import "go.opentelemetry.io/collector/consumer/consumerconnection" |
There was a problem hiding this comment.
consumerconnection go likes shorter names, conn is a good replacement for connection.
Not sure if is not cleaner to have consumer.ConnInfo instead? OR consumerinfo.Conn and consumerinfo.AuthData?
There was a problem hiding this comment.
Updated to consumerconn for now. That gives us consumerconn.Info and consumerconn.AuthData which I like since sharing connection data is the overall theme of the package and Info and AuthData are pieces of connection data the package supports sharing.
| component: collector/client | ||
|
|
||
| # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
| note: Deprecates collector/client. Use `consumerconnection` instead. |
There was a problem hiding this comment.
| note: Deprecates collector/client. Use `consumerconnection` instead. | |
| note: Deprecates collector/client. Use `consumerconn` instead. |
| // Copyright The OpenTelemetry Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| // Package consumerconnection contains generic representations of clients connecting to |
There was a problem hiding this comment.
| // Package consumerconnection contains generic representations of clients connecting to | |
| // Package consumerconn contains generic representations of clients connecting to |
| // receivers: [otlp] | ||
| // processors: [authprinter] | ||
| // exporters: [debug] | ||
| package consumerconn // import "go.opentelemetry.io/collector/consumer/consumerconnection" |
There was a problem hiding this comment.
| package consumerconn // import "go.opentelemetry.io/collector/consumer/consumerconnection" | |
| package consumerconn // import "go.opentelemetry.io/collector/consumer/consumerconn" |
| // Copyright The OpenTelemetry Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| // Package client contains generic representations of clients connecting to |
There was a problem hiding this comment.
| // Package client contains generic representations of clients connecting to | |
| // Package consumerconn contains generic representations of clients connecting to |
| const MetadataHostName = "Host" | ||
|
|
||
| // NewContext takes an existing context and derives a new context with the | ||
| // client.Info value stored on it. | ||
| // | ||
| // Deprecated: [v0.99.0] Use consumerconnection.NewContextWithConnection instead |
There was a problem hiding this comment.
| // Deprecated: [v0.99.0] Use consumerconnection.NewContextWithConnection instead | |
| // Deprecated: [v0.99.0] Use consumerconn.NewContextWithConnection instead |
| @@ -402,7 +402,7 @@ func (hss *ServerConfig) ToServer(_ context.Context, host component.Host, settin | |||
| // TODO: Consider to use component ID string as prefix for all the operations. | |||
| handler = otelhttp.NewHandler(handler, "", otelOpts...) | |||
|
|
|||
| // wrap the current handler in an interceptor that will add client.Info to the request's context | |||
| // wrap the current handler in an interceptor that will add consumerconnection.Info to the request's context | |||
There was a problem hiding this comment.
| // wrap the current handler in an interceptor that will add consumerconnection.Info to the request's context | |
| // wrap the current handler in an interceptor that will add consumerconn.Info to the request's context |
| // | ||
| // Provided that the pipeline does not contain processors that would discard or | ||
| // rewrite the context, such as the batch processor, processors and exporters | ||
| // have access to the consumerconn.Info via consumerconnection.FromContext. Among other usages, |
There was a problem hiding this comment.
| // have access to the consumerconn.Info via consumerconnection.FromContext. Among other usages, | |
| // have access to the consumerconn.Info via consumerconn.FromContext. Among other usages, |
There was a problem hiding this comment.
I'm still not convinced we need this: while this change isn't backwards incompatible and there's a clear migration path, I don't think it's worth the hassle. Every authenticator out there (ours and external) will need to eventually migrate to the new package name. Even if the new name is really great, I don't think we should pursue this or similar changes before v1.
About the Metadata vs. AuthData, we could certainly have a separate discussion about unifying them, but I still see them as serving different purposes: metadata is typically just the headers from HTTP or gRPC, while AuthData doesn't necessarily has to be backed by a map of a slice of strings.
As someone else mentioned on yesterday's call: this package has been stable and serving it's purpose for a long time now, we shouldn't really be changing it in the name of stability for v1.
Description
This PR moves
clientfeatures toconsumerconnection. This attempts to remove ambiguity from the nameclientand continues to centralizes consumer as the location for features that pass data between components.Link to tracking issue
Related to #9818
Testing
Duplicated the unit tests
Documentation
Updated the package comments and added deprecated warnings to the old
clientpackage.