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

[extension/solarwindsapmsettingsextension] Added part one of the concrete implementation of solarwindsapmsettingsextension #30788

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
02589d0
Added config and factory concrete implementation
jerrytfleung Jan 25, 2024
f4a7f61
Added extension function layout
jerrytfleung Jan 25, 2024
0917ef1
Changed interval to time.Duration type and changed ticker
jerrytfleung Jan 25, 2024
a02840e
Updated test cases
jerrytfleung Jan 25, 2024
c14fe3b
Fixed lint
jerrytfleung Jan 25, 2024
de62918
Fixed lint
jerrytfleung Jan 25, 2024
7ab4ef4
Merge branch 'main' into feature/solarwindsapmsettingsextension-concr…
jerrytfleung Jan 25, 2024
6dac276
Added changelog yaml and updated go tidy
jerrytfleung Jan 25, 2024
c3b19ef
Typo
jerrytfleung Jan 25, 2024
840f61d
Merge branch 'main' into feature/solarwindsapmsettingsextension-concr…
jerrytfleung Jan 25, 2024
98553db
Need to make gotidy after rebasing
jerrytfleung Jan 25, 2024
03bde57
Fixed collector version in go.mod
jerrytfleung Jan 25, 2024
63a42e7
Used 10*time.Second
jerrytfleung Jan 29, 2024
c665dc9
Fixed lint problem
jerrytfleung Jan 29, 2024
5f1cd2b
Merge branch 'main' into feature/solarwindsapmsettingsextension-concr…
jerrytfleung Feb 5, 2024
350f9d7
Fixed go.mod
jerrytfleung Feb 5, 2024
8ab2ed8
Merge branch 'main' into feature/solarwindsapmsettingsextension-concr…
jerrytfleung Feb 5, 2024
a975f67
Merge branch 'main' into feature/solarwindsapmsettingsextension-concr…
jerrytfleung Feb 6, 2024
42af1a9
Merge branch 'main' into feature/solarwindsapmsettingsextension-concr…
jerrytfleung Feb 14, 2024
9982165
Updated go.mod
jerrytfleung Feb 14, 2024
204d215
Updated after go mod tidy
jerrytfleung Feb 14, 2024
eba645b
Revert the toolchain version
jerrytfleung Feb 14, 2024
c4ad5e9
extension.go
jerrytfleung Mar 1, 2024
2971a56
Added to call cancel at shutdown to close the ticker loop
jerrytfleung Mar 2, 2024
1a67131
Updated test case
jerrytfleung Mar 4, 2024
9d185f2
Added context.Background()
jerrytfleung Mar 5, 2024
0cef5e7
apm-proto used semver tags
jerrytfleung Apr 2, 2024
80f4820
Updated go.mod & go.sum
jerrytfleung Apr 3, 2024
cf329ce
Merge branch 'main' into feature/solarwindsapmsettingsextension-concr…
jerrytfleung Apr 3, 2024
a489663
Fixed context in Start function
jerrytfleung Apr 3, 2024
20fd9cb
Reverted generated_component_test.go
jerrytfleung Apr 3, 2024
a191fbe
Updated metadata.yaml to pass generated_component_tests
jerrytfleung Apr 3, 2024
e79e2be
nits
jerrytfleung Apr 3, 2024
c4f7ada
Merge branch 'main' into feature/solarwindsapmsettingsextension-concr…
jerrytfleung Apr 5, 2024
c769ae2
nits
jerrytfleung Apr 5, 2024
81bde10
After make gotidy
jerrytfleung Apr 5, 2024
8412a97
Addressed review comments
jerrytfleung Apr 8, 2024
1623736
Merge branch 'main' into feature/solarwindsapmsettingsextension-concr…
jerrytfleung Apr 8, 2024
5218971
Flipped the ordering in Shutdown function
jerrytfleung Apr 10, 2024
00cc4c0
Merge branch 'main' into feature/solarwindsapmsettingsextension-concr…
jerrytfleung Apr 10, 2024
fb67449
Merge branch 'main' into feature/solarwindsapmsettingsextension-concr…
jerrytfleung Apr 17, 2024
0d68961
Updated go.mod
jerrytfleung Apr 17, 2024
64bab4f
Merge branch 'main' into feature/solarwindsapmsettingsextension-concr…
jerrytfleung Apr 17, 2024
a02869e
Merge branch 'main' into feature/solarwindsapmsettingsextension-concr…
jerrytfleung Apr 29, 2024
90b1bbd
Merge branch 'main' into feature/solarwindsapmsettingsextension-concr…
jerrytfleung May 3, 2024
e22a277
Merge branch 'main' into feature/solarwindsapmsettingsextension-concr…
jerrytfleung May 9, 2024
9da1d81
Merge branch 'main' into feature/solarwindsapmsettingsextension-concr…
jerrytfleung May 15, 2024
f2898ed
Merge branch 'main' into feature/solarwindsapmsettingsextension-concr…
jerrytfleung May 16, 2024
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Use this changelog template to create an entry for release notes.

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

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

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Added the first part of concrete implementation of solarwindsapmsettingsextension

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

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:

# 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: []
1 change: 1 addition & 0 deletions cmd/otelcontribcol/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,7 @@ require (
github.com/sirupsen/logrus v1.9.3 // indirect
github.com/snowflakedb/gosnowflake v1.10.1-0.20240509141315-5570db2126fe // indirect
github.com/soheilhy/cmux v0.1.5 // indirect
github.com/solarwindscloud/apm-proto v1.0.3 // indirect
github.com/sourcegraph/conc v0.3.0 // indirect
github.com/spaolacci/murmur3 v1.1.0 // indirect
github.com/spf13/afero v1.11.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions cmd/otelcontribcol/go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 9 additions & 3 deletions extension/solarwindsapmsettingsextension/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,22 @@ extensions:
solarwindsapmsettings:
endpoint: "<endpoint>"
key: "<token>:<name>"
interval: 1m
interval: 10s
```

### endpoint (Required)
The APM collector endpoint which this extension calls `getSettings`. See [here](https://documentation.solarwinds.com/en/success_center/observability/content/system_requirements/endpoints.htm) for our APM collector endpoints.
The APM collector endpoint which this extension calls `getSettings`. See [here](https://documentation.solarwinds.com/en/success_center/observability/content/system_requirements/endpoints.htm) for our APM collector endpoints. The endpoint is in format `<host>:<port>`.

### key (Required)
The service key in format `<token>:<name>` for `getSettings` from Solarwinds APM collector. See [here](https://documentation.solarwinds.com/en/success_center/observability/content/configure/configure-services.htm) for configuring a service key.

### interval (Optional)
Periodic interval to get Solarwinds APM specific settings from Solarwinds APM collector.

Default: `1m`
Minimum value: `5s`

Maximum value: `60s`

Value that is outside the boundary will be bounded to either the minimum or maximum value.

Default: `10s`
71 changes: 50 additions & 21 deletions extension/solarwindsapmsettingsextension/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,38 +4,67 @@
package solarwindsapmsettingsextension // import "github.com/open-telemetry/opentelemetry-collector-contrib/extension/solarwindsapmsettingsextension"

import (
"errors"
"strconv"
"os"
"regexp"
"strings"
"time"

"go.opentelemetry.io/collector/component"
)

type Config struct {
Endpoint string `mapstructure:"endpoint"`
Key string `mapstructure:"key"`
Interval string `mapstructure:"interval"`
Endpoint string `mapstructure:"endpoint"`
Key string `mapstructure:"key"`
Interval time.Duration `mapstructure:"interval"`
}

func (cfg *Config) Validate() error {
Copy link
Member

Choose a reason for hiding this comment

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

Why is validation the removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returning an error in Config.Validate() will cause collector not starting up.

While we are testing with the opentelemetry-lambda collector lambda layer, the collector error from this function can cause the lambda function (user's lambda function) timeout. i.e. A bad config in this extension can break user's lambda function.

While we think a bad config in this extension (e.g. a wrong endpoint causing no such host gRPC error or a missing / wrong key to get a setting should not be a fatal to the collector process.

So we moved the logic to the extension.

When there is a bad config, the extension will log messages and will be in noop.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @jerrytfleung ,

With regards to:

Returning an error in Config.Validate() will cause collector not starting up.

This was a purposeful decision considering that invalid config either means assuming defaults (like a default region, default ingestion endpoint) or letting it run in a broken state (which means it can silently fail)

From my perspective, converting into a noop component on a bad configuration is an anti pattern for a lot of users, a critical piece of infrastructure.

Moreover, without this functionality, the command otelcol validate will return fine which may also contribute to user confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Added back otelcol validate function

if len(cfg.Endpoint) == 0 {
return errors.New("endpoint must not be empty")
}
endpointArr := strings.Split(cfg.Endpoint, ":")
if len(endpointArr) != 2 {
return errors.New("endpoint should be in \"<host>:<port>\" format")
}
if _, err := strconv.Atoi(endpointArr[1]); err != nil {
return errors.New("the <port> portion of endpoint has to be an integer")
const (
DefaultEndpoint = "apm.collector.na-01.cloud.solarwinds.com:443"
DefaultInterval = time.Duration(10) * time.Second
MinimumInterval = time.Duration(5) * time.Second
MaximumInterval = time.Duration(60) * time.Second
)

func createDefaultConfig() component.Config {
return &Config{
Endpoint: DefaultEndpoint,
Interval: DefaultInterval,
}
if len(cfg.Key) == 0 {
return errors.New("key must not be empty")
}

func (cfg *Config) Validate() error {
// Endpoint
matched, _ := regexp.MatchString(`apm.collector.[a-z]{2,3}-[0-9]{2}.[a-z\-]*.solarwinds.com:443`, cfg.Endpoint)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you ever want this to be localhost for testing or other purposes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. We want the endpoint to be one of our APM collector endpoints listed in here
i.e.
apm.collector.na-01.cloud.solarwinds.com:443
apm.collector.na-02.cloud.solarwinds.com:443
apm.collector.eu-01.cloud.solarwinds.com:443
apm.collector.apj-01.cloud.solarwinds.com:443

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only a comment from me and the rest appear fine.

@MovieStoreGuy I fixed the ordering. Can you kindly review again? Thanks!

if !matched {
// Replaced by the default
cfg.Endpoint = DefaultEndpoint
}
// Key
keyArr := strings.Split(cfg.Key, ":")
if len(keyArr) != 2 {
return errors.New("key should be in \"<token>:<service_name>\" format")
if len(keyArr) == 2 && len(keyArr[1]) == 0 {
/**
* Service name is empty. We are trying our best effort to resolve the service name
*/
serviceName := resolveServiceNameBestEffort()
if len(serviceName) > 0 {
cfg.Key = keyArr[0] + ":" + serviceName
}
}
// Interval
if cfg.Interval.Seconds() < MinimumInterval.Seconds() {
cfg.Interval = MinimumInterval
}
if _, err := time.ParseDuration(cfg.Interval); err != nil {
return errors.New("interval has to be a duration string. Valid time units are \"ns\", \"us\" (or \"µs\"), \"ms\", \"s\", \"m\", \"h\"")
if cfg.Interval.Seconds() > MaximumInterval.Seconds() {
cfg.Interval = MaximumInterval
}
return nil
}

func resolveServiceNameBestEffort() string {
if otelServiceName, otelServiceNameDefined := os.LookupEnv("OTEL_SERVICE_NAME"); otelServiceNameDefined && len(otelServiceName) > 0 {
return otelServiceName
} else if awsLambdaFunctionName, awsLambdaFunctionNameDefined := os.LookupEnv("AWS_LAMBDA_FUNCTION_NAME"); awsLambdaFunctionNameDefined && len(awsLambdaFunctionName) > 0 {
return awsLambdaFunctionName
}
return ""
}
98 changes: 60 additions & 38 deletions extension/solarwindsapmsettingsextension/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,63 +4,85 @@
package solarwindsapmsettingsextension

import (
"errors"
"os"
"path/filepath"
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/confmap/confmaptest"

"github.com/open-telemetry/opentelemetry-collector-contrib/extension/solarwindsapmsettingsextension/internal/metadata"
)

func TestValidate(t *testing.T) {
func TestLoadConfig(t *testing.T) {
t.Parallel()

tests := []struct {
name string
cfg *Config
err error
id component.ID
expected component.Config
}{
{
name: "nothing",
cfg: &Config{},
err: errors.New("endpoint must not be empty"),
id: component.NewID(metadata.Type),
expected: NewFactory().CreateDefaultConfig(),
},
{
name: "empty key",
cfg: &Config{
Endpoint: "host:12345",
id: component.NewIDWithName(metadata.Type, "1"),
expected: &Config{
Endpoint: "apm.collector.apj-01.cloud.solarwinds.com:443",
Key: "something:name",
Interval: time.Duration(10) * time.Second,
},
err: errors.New("key must not be empty"),
},
{
name: "invalid endpoint",
cfg: &Config{
Endpoint: "invalid",
Key: "token:name",
id: component.NewIDWithName(metadata.Type, "2"),
expected: &Config{
Endpoint: "apm.collector.na-01.cloud.solarwinds.com:443",
Key: "something",
Interval: time.Duration(5) * time.Second,
},
err: errors.New("endpoint should be in \"<host>:<port>\" format"),
},
{
name: "invalid endpoint format but port is not an integer",
cfg: &Config{
Endpoint: "host:abc",
Key: "token:name",
id: component.NewIDWithName(metadata.Type, "3"),
expected: &Config{
Endpoint: "apm.collector.na-01.cloud.solarwinds.com:443",
Key: "something:name",
Interval: time.Duration(60) * time.Second,
},
err: errors.New("the <port> portion of endpoint has to be an integer"),
},
{
name: "invalid key",
cfg: &Config{
Endpoint: "host:12345",
Key: "invalid",
},
err: errors.New("key should be in \"<token>:<service_name>\" format"),
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
err := tc.cfg.Validate()
if tc.err != nil {
require.EqualError(t, err, tc.err.Error())
} else {
require.NoError(t, err)
}
for _, tt := range tests {
t.Run(tt.id.String(), func(t *testing.T) {
cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config.yaml"))
require.NoError(t, err)
factory := NewFactory()
cfg := factory.CreateDefaultConfig()
sub, err := cm.Sub(tt.id.String())
require.NoError(t, err)
require.NoError(t, component.UnmarshalConfig(sub, cfg))
assert.NoError(t, component.ValidateConfig(cfg))
assert.Equal(t, tt.expected, cfg)
})
}
}

func TestResolveServiceNameBestEffort(t *testing.T) {
// Without any environment variables
require.Empty(t, resolveServiceNameBestEffort())
// With OTEL_SERVICE_NAME only
require.NoError(t, os.Setenv("OTEL_SERVICE_NAME", "otel_ser1"))
require.Equal(t, "otel_ser1", resolveServiceNameBestEffort())
require.NoError(t, os.Unsetenv("OTEL_SERVICE_NAME"))
// With AWS_LAMBDA_FUNCTION_NAME only
require.NoError(t, os.Setenv("AWS_LAMBDA_FUNCTION_NAME", "lambda"))
require.Equal(t, "lambda", resolveServiceNameBestEffort())
require.NoError(t, os.Unsetenv("AWS_LAMBDA_FUNCTION_NAME"))
// With both
require.NoError(t, os.Setenv("OTEL_SERVICE_NAME", "otel_ser1"))
require.NoError(t, os.Setenv("AWS_LAMBDA_FUNCTION_NAME", "lambda"))
require.Equal(t, "otel_ser1", resolveServiceNameBestEffort())
require.NoError(t, os.Unsetenv("AWS_LAMBDA_FUNCTION_NAME"))
require.NoError(t, os.Unsetenv("OTEL_SERVICE_NAME"))
}
50 changes: 47 additions & 3 deletions extension/solarwindsapmsettingsextension/extension.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,23 @@ package solarwindsapmsettingsextension // import "github.com/open-telemetry/open

import (
"context"
"crypto/tls"
"time"

"github.com/solarwindscloud/apm-proto/go/collectorpb"
"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/extension"
"go.uber.org/zap"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials"
)

type solarwindsapmSettingsExtension struct {
logger *zap.Logger
config *Config
cancel context.CancelFunc
conn *grpc.ClientConn
client collectorpb.TraceCollectorClient
}

func newSolarwindsApmSettingsExtension(extensionCfg *Config, logger *zap.Logger) (extension.Extension, error) {
Expand All @@ -26,12 +33,49 @@ func newSolarwindsApmSettingsExtension(extensionCfg *Config, logger *zap.Logger)
}

func (extension *solarwindsapmSettingsExtension) Start(_ context.Context, _ component.Host) error {
extension.logger.Debug("Starting up solarwinds apm settings extension")
_, extension.cancel = context.WithCancel(context.Background())
extension.logger.Info("Starting up solarwinds apm settings extension")
ctx := context.Background()
ctx, extension.cancel = context.WithCancel(ctx)
var err error
extension.conn, err = grpc.Dial(extension.config.Endpoint, grpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{})))
if err != nil {
return err
}
extension.logger.Info("Dailed to endpoint", zap.String("endpoint", extension.config.Endpoint))
extension.client = collectorpb.NewTraceCollectorClient(extension.conn)

// initial refresh
refresh(extension)

go func() {
ticker := time.NewTicker(extension.config.Interval)
defer ticker.Stop()
for {
select {
case <-ticker.C:
refresh(extension)
case <-ctx.Done():
extension.logger.Info("Received ctx.Done() from ticker")
return
}
}
}()

return nil
}

func (extension *solarwindsapmSettingsExtension) Shutdown(_ context.Context) error {
extension.logger.Debug("Shutting down solarwinds apm settings extension")
extension.logger.Info("Shutting down solarwinds apm settings extension")
if extension.cancel != nil {
extension.cancel()
}
if extension.conn != nil {
return extension.conn.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

you're not closing the ticker loop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Followed the tailtracerReceiver example to add a call to the cancel().

Does it address the concern?

Copy link
Contributor

Choose a reason for hiding this comment

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

that works, but you're still exiting early on this line without calling cancel.

Copy link
Contributor

Choose a reason for hiding this comment

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

Flip the ordering and that should resolve the shutdown issue :)

Copy link
Contributor Author

@jerrytfleung jerrytfleung Apr 10, 2024

Choose a reason for hiding this comment

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

Thanks both! :)
Done. I fixed the code.

}
return nil
}

func refresh(extension *solarwindsapmSettingsExtension) {
// Concrete implementation will be available in later PR
extension.logger.Info("refresh task")
}
Loading