-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Changes from all commits
02589d0
f4a7f61
0917ef1
a02840e
c14fe3b
de62918
7ab4ef4
6dac276
c3b19ef
840f61d
98553db
03bde57
63a42e7
c665dc9
5f1cd2b
350f9d7
8ab2ed8
a975f67
42af1a9
9982165
204d215
eba645b
c4ad5e9
2971a56
1a67131
9d185f2
0cef5e7
80f4820
cf329ce
a489663
20fd9cb
a191fbe
e79e2be
c4f7ada
c769ae2
81bde10
8412a97
1623736
5218971
00cc4c0
fb67449
0d68961
64bab4f
a02869e
90b1bbd
e22a277
9da1d81
f2898ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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: [] |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@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 "" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
|
@@ -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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you're not closing the ticker loop There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Followed the tailtracerReceiver example to add a call to the Does it address the concern? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Flip the ordering and that should resolve the shutdown issue :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks both! :) |
||
} | ||
return nil | ||
} | ||
|
||
func refresh(extension *solarwindsapmSettingsExtension) { | ||
// Concrete implementation will be available in later PR | ||
extension.logger.Info("refresh task") | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.There was a problem hiding this comment.
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