Skip to content

Commit 1881912

Browse files
authored
Make all NewDefault* functions return structs instead of pointers (open-telemetry#13169)
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Some `NewDefault*Config` functions return `*Config`, while others return `Config`. As far as I can tell, this is solely because it was easier to write the code for existing usages of the configs. I think that's not the right approach: in my opinion, we should return either always `Config` or always `*Config`, and the former seems to be more natural to me (since the pointer should never be nil)
1 parent ab80fb4 commit 1881912

File tree

8 files changed

+72
-34
lines changed

8 files changed

+72
-34
lines changed
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
# Use this changelog template to create an entry for release notes.
2+
3+
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
4+
change_type: breaking
5+
6+
# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
7+
component: configgrpc,confighttp
8+
9+
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
10+
note: Unify return type of `NewDefault*Config` functions to return a struct instead of a pointer.
11+
12+
# One or more tracking issues or pull requests related to the change
13+
issues: [13169]
14+
15+
# (Optional) One or more lines of additional information to render under the primary note.
16+
# These lines will be padded with 2 spaces and then inserted directly into the document.
17+
# Use pipe (|) for multiline entries.
18+
subtext:
19+
20+
# Optional: The change log or logs in which this entry should be included.
21+
# e.g. '[user]' or '[user, api]'
22+
# Include 'user' if the change is relevant to end users.
23+
# Include 'api' if there is a change to a library API.
24+
# Default: '[user]'
25+
change_logs: [api]

config/configgrpc/configgrpc.go

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,8 @@ type KeepaliveClientConfig struct {
5252
}
5353

5454
// NewDefaultKeepaliveClientConfig returns a new instance of KeepaliveClientConfig with default values.
55-
func NewDefaultKeepaliveClientConfig() *KeepaliveClientConfig {
56-
return &KeepaliveClientConfig{
55+
func NewDefaultKeepaliveClientConfig() KeepaliveClientConfig {
56+
return KeepaliveClientConfig{
5757
Time: time.Second * 10,
5858
Timeout: time.Second * 10,
5959
}
@@ -111,11 +111,15 @@ type ClientConfig struct {
111111
Middlewares []configmiddleware.Config `mapstructure:"middlewares,omitempty"`
112112
}
113113

114+
func ptr[T any](v T) *T {
115+
return &v
116+
}
117+
114118
// NewDefaultClientConfig returns a new instance of ClientConfig with default values.
115-
func NewDefaultClientConfig() *ClientConfig {
116-
return &ClientConfig{
119+
func NewDefaultClientConfig() ClientConfig {
120+
return ClientConfig{
117121
TLS: configtls.NewDefaultClientConfig(),
118-
Keepalive: NewDefaultKeepaliveClientConfig(),
122+
Keepalive: ptr(NewDefaultKeepaliveClientConfig()),
119123
BalancerName: BalancerName(),
120124
}
121125
}
@@ -129,10 +133,10 @@ type KeepaliveServerConfig struct {
129133
}
130134

131135
// NewDefaultKeepaliveServerConfig returns a new instance of KeepaliveServerConfig with default values.
132-
func NewDefaultKeepaliveServerConfig() *KeepaliveServerConfig {
133-
return &KeepaliveServerConfig{
134-
ServerParameters: NewDefaultKeepaliveServerParameters(),
135-
EnforcementPolicy: NewDefaultKeepaliveEnforcementPolicy(),
136+
func NewDefaultKeepaliveServerConfig() KeepaliveServerConfig {
137+
return KeepaliveServerConfig{
138+
ServerParameters: ptr(NewDefaultKeepaliveServerParameters()),
139+
EnforcementPolicy: ptr(NewDefaultKeepaliveEnforcementPolicy()),
136140
}
137141
}
138142

@@ -150,8 +154,8 @@ type KeepaliveServerParameters struct {
150154
}
151155

152156
// NewDefaultKeepaliveServerParameters creates and returns a new instance of KeepaliveServerParameters with default settings.
153-
func NewDefaultKeepaliveServerParameters() *KeepaliveServerParameters {
154-
return &KeepaliveServerParameters{}
157+
func NewDefaultKeepaliveServerParameters() KeepaliveServerParameters {
158+
return KeepaliveServerParameters{}
155159
}
156160

157161
// KeepaliveEnforcementPolicy allow configuration of the keepalive.EnforcementPolicy.
@@ -165,8 +169,8 @@ type KeepaliveEnforcementPolicy struct {
165169
}
166170

167171
// NewDefaultKeepaliveEnforcementPolicy creates and returns a new instance of KeepaliveEnforcementPolicy with default settings.
168-
func NewDefaultKeepaliveEnforcementPolicy() *KeepaliveEnforcementPolicy {
169-
return &KeepaliveEnforcementPolicy{}
172+
func NewDefaultKeepaliveEnforcementPolicy() KeepaliveEnforcementPolicy {
173+
return KeepaliveEnforcementPolicy{}
170174
}
171175

172176
// ServerConfig defines common settings for a gRPC server configuration.
@@ -210,9 +214,9 @@ type ServerConfig struct {
210214
}
211215

212216
// NewDefaultServerConfig returns a new instance of ServerConfig with default values.
213-
func NewDefaultServerConfig() *ServerConfig {
214-
return &ServerConfig{
215-
Keepalive: NewDefaultKeepaliveServerConfig(),
217+
func NewDefaultServerConfig() ServerConfig {
218+
return ServerConfig{
219+
Keepalive: ptr(NewDefaultKeepaliveServerConfig()),
216220
}
217221
}
218222

config/configgrpc/configgrpc_test.go

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ func newMockAuthServer(auth func(ctx context.Context, sources map[string][]strin
5151
}
5252

5353
func TestNewDefaultKeepaliveClientConfig(t *testing.T) {
54-
expectedKeepaliveClientConfig := &KeepaliveClientConfig{
54+
expectedKeepaliveClientConfig := KeepaliveClientConfig{
5555
Time: time.Second * 10,
5656
Timeout: time.Second * 10,
5757
}
@@ -60,9 +60,10 @@ func TestNewDefaultKeepaliveClientConfig(t *testing.T) {
6060
}
6161

6262
func TestNewDefaultClientConfig(t *testing.T) {
63-
expected := &ClientConfig{
63+
keepalive := NewDefaultKeepaliveClientConfig()
64+
expected := ClientConfig{
6465
TLS: configtls.NewDefaultClientConfig(),
65-
Keepalive: NewDefaultKeepaliveClientConfig(),
66+
Keepalive: &keepalive,
6667
BalancerName: BalancerName(),
6768
}
6869

@@ -72,32 +73,32 @@ func TestNewDefaultClientConfig(t *testing.T) {
7273
}
7374

7475
func TestNewDefaultKeepaliveServerParameters(t *testing.T) {
75-
expectedParams := &KeepaliveServerParameters{}
76+
expectedParams := KeepaliveServerParameters{}
7677
params := NewDefaultKeepaliveServerParameters()
7778

7879
assert.Equal(t, expectedParams, params)
7980
}
8081

8182
func TestNewDefaultKeepaliveEnforcementPolicy(t *testing.T) {
82-
expectedPolicy := &KeepaliveEnforcementPolicy{}
83+
expectedPolicy := KeepaliveEnforcementPolicy{}
8384

8485
policy := NewDefaultKeepaliveEnforcementPolicy()
8586

8687
assert.Equal(t, expectedPolicy, policy)
8788
}
8889

8990
func TestNewDefaultKeepaliveServerConfig(t *testing.T) {
90-
expected := &KeepaliveServerConfig{
91-
ServerParameters: NewDefaultKeepaliveServerParameters(),
92-
EnforcementPolicy: NewDefaultKeepaliveEnforcementPolicy(),
91+
expected := KeepaliveServerConfig{
92+
ServerParameters: ptr(NewDefaultKeepaliveServerParameters()),
93+
EnforcementPolicy: ptr(NewDefaultKeepaliveEnforcementPolicy()),
9394
}
9495
result := NewDefaultKeepaliveServerConfig()
9596
assert.Equal(t, expected, result)
9697
}
9798

9899
func TestNewDefaultServerConfig(t *testing.T) {
99-
expected := &ServerConfig{
100-
Keepalive: NewDefaultKeepaliveServerConfig(),
100+
expected := ServerConfig{
101+
Keepalive: ptr(NewDefaultKeepaliveServerConfig()),
101102
}
102103

103104
result := NewDefaultServerConfig()

config/confighttp/confighttp.go

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -373,12 +373,16 @@ type ServerConfig struct {
373373
Middlewares []configmiddleware.Config `mapstructure:"middleware,omitempty"`
374374
}
375375

376+
func ptr[T any](v T) *T {
377+
return &v
378+
}
379+
376380
// NewDefaultServerConfig returns ServerConfig type object with default values.
377381
// We encourage to use this function to create an object of ServerConfig.
378382
func NewDefaultServerConfig() ServerConfig {
379383
return ServerConfig{
380384
ResponseHeaders: map[string]configopaque.String{},
381-
CORS: NewDefaultCORSConfig(),
385+
CORS: ptr(NewDefaultCORSConfig()),
382386
WriteTimeout: 30 * time.Second,
383387
ReadHeaderTimeout: 1 * time.Minute,
384388
IdleTimeout: 1 * time.Minute,
@@ -594,8 +598,8 @@ type CORSConfig struct {
594598
}
595599

596600
// NewDefaultCORSConfig creates a default cross-origin resource sharing (CORS) configuration.
597-
func NewDefaultCORSConfig() *CORSConfig {
598-
return &CORSConfig{}
601+
func NewDefaultCORSConfig() CORSConfig {
602+
return CORSConfig{}
599603
}
600604

601605
func authInterceptor(next http.Handler, server extensionauth.Server, requestParams []string, serverOpts *internal.ToServerOptions) http.Handler {

config/confighttp/confighttp_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -740,7 +740,7 @@ func TestHttpCors(t *testing.T) {
740740
},
741741
{
742742
name: "emptyCORS",
743-
CORSConfig: NewDefaultCORSConfig(),
743+
CORSConfig: ptr(NewDefaultCORSConfig()),
744744
allowedWorks: false,
745745
disallowedWorks: false,
746746
extraHeaderWorks: false,

exporter/otlpexporter/factory.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ func NewFactory() exporter.Factory {
3131
}
3232

3333
func createDefaultConfig() component.Config {
34-
clientCfg := *configgrpc.NewDefaultClientConfig()
34+
clientCfg := configgrpc.NewDefaultClientConfig()
3535
// Default to gzip compression
3636
clientCfg.Compression = configcompression.TypeGzip
3737
// We almost read 0 bytes, so no need to tune ReadBufferSize.

receiver/otlpreceiver/config_test.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,10 @@ func TestUnmarshalConfig(t *testing.T) {
163163
}, cfg)
164164
}
165165

166+
func ptr[T any](v T) *T {
167+
return &v
168+
}
169+
166170
func TestUnmarshalConfigUnix(t *testing.T) {
167171
cm, err := confmaptest.LoadConf(filepath.Join("testdata", "uds.yaml"))
168172
require.NoError(t, err)
@@ -178,12 +182,12 @@ func TestUnmarshalConfigUnix(t *testing.T) {
178182
Transport: confignet.TransportTypeUnix,
179183
},
180184
ReadBufferSize: 512 * 1024,
181-
Keepalive: configgrpc.NewDefaultKeepaliveServerConfig(),
185+
Keepalive: ptr(configgrpc.NewDefaultKeepaliveServerConfig()),
182186
}),
183187
HTTP: configoptional.Some(HTTPConfig{
184188
ServerConfig: confighttp.ServerConfig{
185189
Endpoint: "/tmp/http_otlp.sock",
186-
CORS: confighttp.NewDefaultCORSConfig(),
190+
CORS: ptr(confighttp.NewDefaultCORSConfig()),
187191
ResponseHeaders: map[string]configopaque.String{},
188192
},
189193
TracesURLPath: defaultTracesURLPath,

receiver/otlpreceiver/factory.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ func createDefaultConfig() component.Config {
5757

5858
return &Config{
5959
Protocols: Protocols{
60-
GRPC: configoptional.Default(*grpcCfg),
60+
GRPC: configoptional.Default(grpcCfg),
6161
HTTP: configoptional.Default(HTTPConfig{
6262
ServerConfig: httpCfg,
6363
TracesURLPath: defaultTracesURLPath,

0 commit comments

Comments
 (0)