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 12 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 @@ -614,6 +614,7 @@ require (
github.com/sirupsen/logrus v1.9.3 // indirect
github.com/snowflakedb/gosnowflake v1.7.2 // indirect
github.com/soheilhy/cmux v0.1.5 // indirect
github.com/solarwindscloud/apm-proto v0.0.0-20231107001908-432e697887b6 // 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`
38 changes: 4 additions & 34 deletions extension/solarwindsapmsettingsextension/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,40 +2,10 @@
// SPDX-License-Identifier: Apache-2.0

package solarwindsapmsettingsextension // import "github.com/open-telemetry/opentelemetry-collector-contrib/extension/solarwindsapmsettingsextension"

import (
"errors"
"strconv"
"strings"
"time"
)
import "time"

type Config struct {
Endpoint string `mapstructure:"endpoint"`
Key string `mapstructure:"key"`
Interval string `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")
}
if len(cfg.Key) == 0 {
return errors.New("key must not be empty")
}
keyArr := strings.Split(cfg.Key, ":")
if len(keyArr) != 2 {
return errors.New("key should be in \"<token>:<service_name>\" format")
}
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\"")
}
return nil
Endpoint string `mapstructure:"endpoint"`
Key string `mapstructure:"key"`
Interval time.Duration `mapstructure:"interval"`
}
76 changes: 35 additions & 41 deletions extension/solarwindsapmsettingsextension/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,63 +4,57 @@
package solarwindsapmsettingsextension

import (
"errors"
"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"),
},
{
name: "empty key",
cfg: &Config{
Endpoint: "host:12345",
},
err: errors.New("key must not be empty"),
},
{
name: "invalid endpoint",
cfg: &Config{
Endpoint: "invalid",
Key: "token:name",
},
err: errors.New("endpoint should be in \"<host>:<port>\" format"),
id: component.NewID(metadata.Type),
expected: NewFactory().CreateDefaultConfig(),
},
{
name: "invalid endpoint format but port is not an integer",
cfg: &Config{
Endpoint: "host:abc",
Key: "token:name",
id: component.NewIDWithName(metadata.Type, "1"),
expected: &Config{
Endpoint: "0.0.0.0:1234",
Key: "something",
Interval: time.Duration(10000000000),
},
err: errors.New("the <port> portion of endpoint has to be an integer"),
},
{
name: "invalid key",
cfg: &Config{
Endpoint: "host:12345",
Key: "invalid",
id: component.NewIDWithName(metadata.Type, "2"),
expected: &Config{
Endpoint: "0.0.0.0:1234",
Key: "something",
Interval: time.Duration(10000000000),
},
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)
})
}
}
61 changes: 59 additions & 2 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 @@ -25,13 +32,63 @@ func newSolarwindsApmSettingsExtension(extensionCfg *Config, logger *zap.Logger)
return settingsExtension, nil
}

func (extension *solarwindsapmSettingsExtension) Start(_ context.Context, _ component.Host) error {
func (extension *solarwindsapmSettingsExtension) Start(ctx context.Context, _ component.Host) error {
extension.logger.Debug("Starting up solarwinds apm settings extension")
_, extension.cancel = context.WithCancel(context.Background())
ctx, extension.cancel = context.WithCancel(ctx)
configOk := validateSolarwindsApmSettingsExtensionConfiguration(extension.config, extension.logger)
if configOk {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest inverting the condition here to avoid such a nested function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the condition. Done

extension.conn, _ = grpc.Dial(extension.config.Endpoint, grpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{})))
Copy link
Contributor

Choose a reason for hiding this comment

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

you are ignoring the error returned by this function.

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 fixed it by returning the error.

extension.logger.Info("Dailed to " + extension.config.Endpoint)
atoulme marked this conversation as resolved.
Show resolved Hide resolved
extension.client = collectorpb.NewTraceCollectorClient(extension.conn)
go func() {
ticker := newTicker(extension.config.Interval)
defer ticker.Stop()
for {
select {
case <-ticker.C:
refresh(extension)
case <-ctx.Done():
jerrytfleung marked this conversation as resolved.
Show resolved Hide resolved
extension.logger.Debug("Received ctx.Done() from ticker")
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not typically how a component is stopped.

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 was referring to the tailtracerReceiver example to implement the extension.
Can you help pointing me a typical example to stop a component?

return
}
}
}()
} else {
extension.logger.Warn("solarwindsapmsettingsextension is in noop. Please check config")
Copy link
Contributor

Choose a reason for hiding this comment

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

I strongly advise against this, I would assume a majority of users would miss this log line since reaching this point has meant the collector has done all the expected checks and validations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the noop scope. Done.

}
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed. Config.Validate() is called by the builder automatically and collector doesn't start with returned error message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but it can break user's application when it is used in collector lambda layer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but it can break user's application when it is used in collector lambda layer.

The time couple of lambda to lambda extensions can be painful, I am not sure it justifies removing all protections. I would recommend that you raise that the lambda project as a feature in order to allow the lambda layer to run without error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I will raise that to lambda project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MovieStoreGuy We are thinking to add back Config.Validate() and replace any invalid field of the configuration by default values. As we set default values, Config.validate() will always return nil. And the extension will not need to work in noop mode anymore.

What do you think?

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 back Config.Validate()

return nil
}

func (extension *solarwindsapmSettingsExtension) Shutdown(_ context.Context) error {
extension.logger.Debug("Shutting down solarwinds apm settings extension")
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 validateSolarwindsApmSettingsExtensionConfiguration(_ *Config, _ *zap.Logger) bool {
// Concrete implementation will be available in later PR
return true
}

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

// Start ticking immediately.
// Ref: https://stackoverflow.com/questions/32705582/how-to-get-time-tick-to-tick-immediately
func newTicker(repeat time.Duration) *time.Ticker {
ticker := time.NewTicker(repeat)
oc := ticker.C
nc := make(chan time.Time, 1)
go func() {
nc <- time.Now()
for tm := range oc {
nc <- tm
}
}()
ticker.C = nc
return ticker
}
Copy link
Contributor

Choose a reason for hiding this comment

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

An easier to do this is switch the loop from being a For each tick, do this to For step, do and wait, ie:

// Do step and wait 
for { 
  // action 
  select {
  case <-ctx.Done():
  case <- ticker.C:
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

49 changes: 25 additions & 24 deletions extension/solarwindsapmsettingsextension/extension_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,40 +6,41 @@ package solarwindsapmsettingsextension
import (
"context"
"testing"
"time"

"github.com/stretchr/testify/require"
"go.opentelemetry.io/collector/extension"
"go.uber.org/zap"
)

func TestCreateExtension(t *testing.T) {
conf := &Config{
Endpoint: "apm-testcollector.click:443",
Key: "valid:unittest",
Interval: "1s",
}
ex := createAnExtension(conf, t)
require.NoError(t, ex.Shutdown(context.TODO()))
}
t.Parallel()

func TestCreateExtensionWrongEndpoint(t *testing.T) {
conf := &Config{
Endpoint: "apm-testcollector.nothing:443",
Key: "valid:unittest",
Interval: "1s",
tests := []struct {
name string
cfg *Config
}{
{
name: "default",
cfg: &Config{
Interval: time.Duration(10000000000),
},
},
{
name: "anything",
cfg: &Config{
Endpoint: "0.0.0.0:1234",
Key: "something",
Interval: time.Duration(10000000000),
},
},
}
ex := createAnExtension(conf, t)
require.NoError(t, ex.Shutdown(context.TODO()))
}

func TestCreateExtensionWrongKey(t *testing.T) {
conf := &Config{
Endpoint: "apm-testcollector.click:443",
Key: "invalid",
Interval: "1s",
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ex := createAnExtension(tt.cfg, t)
require.NoError(t, ex.Shutdown(context.TODO()))
})
}
ex := createAnExtension(conf, t)
require.NoError(t, ex.Shutdown(context.TODO()))
}

// create extension
Expand Down
3 changes: 2 additions & 1 deletion extension/solarwindsapmsettingsextension/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package solarwindsapmsettingsextension // import "github.com/open-telemetry/open

import (
"context"
"time"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/extension"
Expand All @@ -13,7 +14,7 @@ import (
)

const (
DefaultInterval = "1m"
DefaultInterval = time.Duration(10000000000)
Copy link
Member

Choose a reason for hiding this comment

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

Is it 10*time.Second?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks!
Fixed.

)

func createDefaultConfig() component.Config {
Expand Down
Loading
Loading