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

[carbonreceiver] Hide unnecessary public API #29895

Merged
merged 1 commit into from
Dec 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Hide unnecessary public API
Signed-off-by: Bogdan Drutu <[email protected]>
  • Loading branch information
bogdandrutu committed Dec 15, 2023
commit e1fe07ce535be4f85600aca91cf72c0a4862e1db
22 changes: 22 additions & 0 deletions .chloggen/carbonreceiver.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: 'breaking'

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: "carbonreceiver"

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Hide unnecessary public API

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [29895]

# If your change doesn't affect end users or the exported elements of any package,
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: [api]
41 changes: 7 additions & 34 deletions receiver/carbonreceiver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,16 @@
package carbonreceiver // import "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver"

import (
"fmt"
"errors"
"time"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/config/confignet"
"go.opentelemetry.io/collector/confmap"

"github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver/protocol"
)

const (
// parserConfigSection is the name that must be used for the parser settings
// in the configuration struct. The metadata mapstructure for the parser
// should use the same string.
parserConfigSection = "parser"
)

var _ confmap.Unmarshaler = (*Config)(nil)
var _ component.ConfigValidator = (*Config)(nil)

// Config defines configuration for the Carbon receiver.
type Config struct {
Expand All @@ -35,29 +28,9 @@ type Config struct {
Parser *protocol.Config `mapstructure:"parser"`
}

func (cfg *Config) Unmarshal(componentParser *confmap.Conf) error {
if componentParser == nil {
// The section is empty nothing to do, using the default config.
return nil
func (cfg *Config) Validate() error {
if cfg.TCPIdleTimeout < 0 {
return errors.New("'tcp_idle_timeout' must be non-negative")
}

// Unmarshal but not exact yet so the different keys under config do not
// trigger errors, this is needed so that the types of protocol and transport
// are read.
if err := componentParser.Unmarshal(cfg); err != nil {
return err
}

// Unmarshal the protocol, so the type of config can be properly set.
vParserCfg, errSub := componentParser.Sub(parserConfigSection)
if errSub != nil {
return errSub
}

if err := protocol.LoadParserConfig(vParserCfg, cfg.Parser); err != nil {
return fmt.Errorf("error on %q section: %w", parserConfigSection, err)
}

// Unmarshal exact to validate the config keys.
return componentParser.Unmarshal(cfg, confmap.WithErrorUnused())
return nil
}
15 changes: 15 additions & 0 deletions receiver/carbonreceiver/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,3 +92,18 @@ func TestLoadConfig(t *testing.T) {
})
}
}

func TestConfigValidate(t *testing.T) {
cfg := &Config{
NetAddr: confignet.NetAddr{
Endpoint: "localhost:2003",
Transport: "tcp",
},
TCPIdleTimeout: -1 * time.Second,
Parser: &protocol.Config{
Type: "plaintext",
Config: &protocol.PlaintextConfig{},
},
}
assert.Error(t, cfg.Validate())
}
9 changes: 7 additions & 2 deletions receiver/carbonreceiver/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package carbonreceiver // import "github.com/open-telemetry/opentelemetry-collec

import (
"context"
"time"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/config/confignet"
Expand All @@ -13,7 +14,11 @@ import (

"github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver/internal/metadata"
"github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver/protocol"
"github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver/transport"
)

const (
// tcpIdleTimeoutDefault is the default timeout for idle TCP connections.
tcpIdleTimeoutDefault = 30 * time.Second
)

// This file implements factory for Carbon receiver.
Expand All @@ -32,7 +37,7 @@ func createDefaultConfig() component.Config {
Endpoint: "localhost:2003",
Transport: "tcp",
},
TCPIdleTimeout: transport.TCPIdleTimeoutDefault,
TCPIdleTimeout: tcpIdleTimeoutDefault,
Parser: &protocol.Config{
Type: "plaintext",
Config: &protocol.PlaintextConfig{},
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

package client // import "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver/transport/client"
package client // import "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver/internal/client"

import (
"fmt"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

package transport // import "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver/transport"
package transport // import "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver/internal/transport"

import (
"context"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package transport

import (
"context"
"runtime"
"sync"
"testing"
Expand All @@ -14,8 +15,8 @@ import (
"go.opentelemetry.io/collector/consumer/consumertest"

"github.com/open-telemetry/opentelemetry-collector-contrib/internal/common/testutil"
"github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver/internal/client"
"github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver/protocol"
"github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver/transport/client"
)

func Test_Server_ListenAndServe(t *testing.T) {
Expand Down Expand Up @@ -52,7 +53,8 @@ func Test_Server_ListenAndServe(t *testing.T) {
mc := new(consumertest.MetricsSink)
p, err := (&protocol.PlaintextConfig{}).BuildParser()
require.NoError(t, err)
mr := NewMockReporter(1)
mr := &mockReporter{}
mr.wgMetricsProcessed.Add(1)

wgListenAndServe := sync.WaitGroup{}
wgListenAndServe.Add(1)
Expand All @@ -76,7 +78,7 @@ func Test_Server_ListenAndServe(t *testing.T) {
err = gc.Disconnect()
assert.NoError(t, err)

mr.WaitAllOnMetricsProcessedCalls()
mr.wgMetricsProcessed.Wait()

// Keep trying until we're timed out or got a result
assert.Eventually(t, func() bool {
Expand All @@ -96,3 +98,21 @@ func Test_Server_ListenAndServe(t *testing.T) {
})
}
}

// mockReporter provides a Reporter that provides some useful functionalities for
// tests (eg.: wait for certain number of messages).
type mockReporter struct {
wgMetricsProcessed sync.WaitGroup
}

func (m *mockReporter) OnDataReceived(ctx context.Context) context.Context {
return ctx
}

func (m *mockReporter) OnTranslationError(context.Context, error) {}

func (m *mockReporter) OnMetricsProcessed(context.Context, int, error) {
m.wgMetricsProcessed.Done()
}

func (m *mockReporter) OnDebugf(string, ...any) {}
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

package transport // import "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver/transport"
package transport // import "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver/internal/transport"

import (
"bufio"
"context"
"errors"
"fmt"
"io"
"net"
"strings"
Expand All @@ -20,11 +19,6 @@ import (
"github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver/protocol"
)

const (
// TCPIdleTimeoutDefault is the default timeout for idle TCP connections.
TCPIdleTimeoutDefault = 30 * time.Second
)

type tcpServer struct {
ln net.Listener
wg sync.WaitGroup
Expand All @@ -39,14 +33,6 @@ func NewTCPServer(
addr string,
idleTimeout time.Duration,
) (Server, error) {
if idleTimeout < 0 {
return nil, fmt.Errorf("invalid idle timeout: %v", idleTimeout)
}

if idleTimeout == 0 {
idleTimeout = TCPIdleTimeoutDefault
}

ln, err := net.Listen("tcp", addr)
if err != nil {
return nil, err
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

package transport // import "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver/transport"
package transport // import "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver/internal/transport"

import (
"bytes"
Expand Down
26 changes: 10 additions & 16 deletions receiver/carbonreceiver/protocol/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,7 @@ func init() {
})
}

const (
// configSection is the name that must be used for the config settings
// in the configuration struct. The metadata mapstructure for the parser
// should use the same string.
configSection = "config"
)
var _ confmap.Unmarshaler = (*Config)(nil)

// Config is the general configuration for the parser to be used.
type Config struct {
Expand All @@ -57,10 +52,14 @@ type ParserConfig interface {
BuildParser() (Parser, error)
}

// LoadParserConfig is used to load the parser configuration according to the
// specified parser type. It expects the passed viper to be pointing at the level
// of the Config reference.
func LoadParserConfig(cp *confmap.Conf, cfg *Config) error {
// Unmarshal is used to load the parser configuration according to the
// specified parser type.
func (cfg *Config) Unmarshal(cp *confmap.Conf) error {
// If type is configured then use that, otherwise use default.
if configuredType, ok := cp.Get("type").(string); ok {
cfg.Type = configuredType
}

defaultCfgFn, ok := parserMap[cfg.Type]
if !ok {
return fmt.Errorf(
Expand All @@ -71,10 +70,5 @@ func LoadParserConfig(cp *confmap.Conf, cfg *Config) error {

cfg.Config = defaultCfgFn()

vParserCfg, errSub := cp.Sub(configSection)
if errSub != nil {
return errSub
}

return vParserCfg.Unmarshal(cfg.Config, confmap.WithErrorUnused())
return cp.Unmarshal(cfg, confmap.WithErrorUnused())
}
18 changes: 11 additions & 7 deletions receiver/carbonreceiver/protocol/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,12 @@ func TestLoadParserConfig(t *testing.T) {
tests := []struct {
name string
cfgMap map[string]any
cfg Config
want Config
wantErr bool
}{
{
name: "unknow_type",
cfgMap: map[string]any{"type": "unknow"},
cfg: Config{Type: "unknown"},
cfgMap: map[string]any{"type": "unknown"},
want: Config{Type: "unknown"},
wantErr: true,
},
Expand All @@ -35,7 +33,6 @@ func TestLoadParserConfig(t *testing.T) {
"rules": []any{map[string]any{"regexp": "(?<key_test>.*test)"}},
},
},
cfg: Config{Type: "regex"},
want: Config{
Type: "regex",
Config: &RegexParserConfig{
Expand All @@ -47,19 +44,26 @@ func TestLoadParserConfig(t *testing.T) {
{
name: "default_regex",
cfgMap: map[string]any{"type": "regex"},
cfg: Config{Type: "regex"},
want: Config{
Type: "regex",
Config: &RegexParserConfig{},
},
},
{
name: "plaintext",
cfgMap: map[string]any{"type": "plaintext"},
want: Config{
Type: "plaintext",
Config: &PlaintextConfig{},
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
v := confmap.NewFromStringMap(tt.cfgMap)

got := tt.cfg // Not strictly necessary but it makes easier to debug issues.
err := LoadParserConfig(v, &got)
got := Config{}
err := got.Unmarshal(v)
assert.Equal(t, tt.want, got)
assert.Equal(t, tt.wantErr, err != nil)
})
Expand Down
2 changes: 1 addition & 1 deletion receiver/carbonreceiver/receiver.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ import (
"go.opentelemetry.io/collector/consumer"
"go.opentelemetry.io/collector/receiver"

"github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver/internal/transport"
"github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver/protocol"
"github.com/open-telemetry/opentelemetry-collector-contrib/receiver/carbonreceiver/transport"
)

var (
Expand Down
Loading
Loading