-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 22 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 |
---|---|---|
|
@@ -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) { | ||
|
@@ -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 { | ||
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. I would suggest inverting the condition here to avoid such a nested function 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. Removed the condition. Done |
||
extension.conn, _ = grpc.Dial(extension.config.Endpoint, grpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{}))) | ||
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 are ignoring the error returned by this function. 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. 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") | ||
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. this is not typically how a component is stopped. 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. I was referring to the tailtracerReceiver example to implement the extension. |
||
return | ||
} | ||
} | ||
}() | ||
} else { | ||
extension.logger.Warn("solarwindsapmsettingsextension is in noop. Please check config") | ||
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. 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. 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. Removed the noop scope. Done. |
||
} | ||
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. This is not needed. 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. Yes, but it can break user's application when it is used in collector lambda layer. 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.
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. 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. Sure. I will raise that to lambda project. 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. Raised a feature request 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. @MovieStoreGuy We are thinking to add back What do you think? 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. Added back |
||
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() | ||
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 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 | ||
} | ||
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. 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:
}
} 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. Referenced https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/receiver/awscontainerinsightreceiver/internal/host/utils.go#L41 to add initial refresh and use |
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