-
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 remaining implementation of solarwindsapmsettingsextension #33315
Changes from 30 commits
d53d9da
f4258b1
f24f425
f4c6d03
4674c76
d1cba82
a0688c5
07212ee
08dc77b
baa26f9
0084eac
465c376
0120d04
172e950
8f65ad1
85091f5
0ef422b
d75baae
d471abb
4c61613
ca529fb
005f428
ce61f50
25be1fd
7b92892
080daef
7119763
1b59c85
a2ee3fa
da99852
2a17e91
78f38c0
59b1911
5c78537
0b1330b
574d3f0
5bb4ddc
82ef634
3f31f51
cdeee9f
9475c30
398f3f4
096ba0d
45ce744
630f1ce
a55f64e
a47f1c1
33570e1
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 logic for refresh function | ||
|
||
# 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 |
---|---|---|
|
@@ -6,16 +6,26 @@ package solarwindsapmsettingsextension // import "github.com/open-telemetry/open | |
import ( | ||
"context" | ||
"crypto/tls" | ||
"crypto/x509" | ||
"encoding/binary" | ||
"encoding/json" | ||
"math" | ||
"os" | ||
"time" | ||
|
||
"github.com/solarwindscloud/apm-proto/go/collectorpb" | ||
"github.com/solarwinds/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" | ||
) | ||
|
||
const ( | ||
jsonOutputFile = "/tmp/solarwinds-apm-settings.json" | ||
grpcContextDeadline = 1 * time.Second | ||
) | ||
|
||
type solarwindsapmSettingsExtension struct { | ||
logger *zap.Logger | ||
config *Config | ||
|
@@ -33,29 +43,32 @@ func newSolarwindsApmSettingsExtension(extensionCfg *Config, logger *zap.Logger) | |
} | ||
|
||
func (extension *solarwindsapmSettingsExtension) Start(_ context.Context, _ component.Host) error { | ||
extension.logger.Info("Starting up solarwinds apm settings extension") | ||
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.NewClient(extension.config.Endpoint, grpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{}))) | ||
systemCertPool, err := x509.SystemCertPool() | ||
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 added code to get the system certificates pool for TLS configuration. 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 suggest reusing the configuration and helpers that are provided in https://github.com/open-telemetry/opentelemetry-collector/tree/main/config/configgrpc , saves a decent amount of effort and reduces the risk of doing it wrong. 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. Done. It is a good idea. Thanks! |
||
if err != nil { | ||
return err | ||
} | ||
extension.logger.Info("Dailed to endpoint", zap.String("endpoint", extension.config.Endpoint)) | ||
extension.conn, err = grpc.NewClient(extension.config.Endpoint, grpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{RootCAs: systemCertPool}))) | ||
if err != nil { | ||
return err | ||
} | ||
extension.logger.Info("created a gRPC client", zap.String("endpoint", extension.config.Endpoint)) | ||
extension.client = collectorpb.NewTraceCollectorClient(extension.conn) | ||
|
||
// initial refresh | ||
refresh(extension) | ||
refresh(extension, jsonOutputFile) | ||
|
||
go func() { | ||
ticker := time.NewTicker(extension.config.Interval) | ||
defer ticker.Stop() | ||
for { | ||
select { | ||
case <-ticker.C: | ||
refresh(extension) | ||
refresh(extension, jsonOutputFile) | ||
case <-ctx.Done(): | ||
extension.logger.Info("Received ctx.Done() from ticker") | ||
extension.logger.Info("received ctx.Done() from ticker") | ||
return | ||
} | ||
} | ||
|
@@ -65,7 +78,7 @@ func (extension *solarwindsapmSettingsExtension) Start(_ context.Context, _ comp | |
} | ||
|
||
func (extension *solarwindsapmSettingsExtension) Shutdown(_ context.Context) error { | ||
extension.logger.Info("Shutting down solarwinds apm settings extension") | ||
extension.logger.Info("shutting down solarwinds apm settings extension") | ||
if extension.cancel != nil { | ||
extension.cancel() | ||
} | ||
|
@@ -75,7 +88,93 @@ func (extension *solarwindsapmSettingsExtension) Shutdown(_ context.Context) err | |
return nil | ||
} | ||
|
||
func refresh(extension *solarwindsapmSettingsExtension) { | ||
// Concrete implementation will be available in later PR | ||
extension.logger.Info("refresh task") | ||
func refresh(extension *solarwindsapmSettingsExtension, filename string) { | ||
extension.logger.Info("time to refresh", zap.String("endpoint", extension.config.Endpoint)) | ||
if hostname, err := os.Hostname(); err != nil { | ||
extension.logger.Error("unable to call os.Hostname()", zap.Error(err)) | ||
} else { | ||
ctx, cancel := context.WithTimeout(context.Background(), grpcContextDeadline) | ||
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 passing context in or storing some async state that you can tell if you're meant to shutdown so it doesn't either leak routines or blocks un-neededly. It is a lot easier to pass in the context 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. The context is for the gRPC request to Solarwinds APM backend. We don't want any context timeout / cancel of the request causing any cancellation / error to collector as we can always refresh in the next cycle. |
||
defer cancel() | ||
|
||
request := &collectorpb.SettingsRequest{ | ||
ApiKey: extension.config.Key, | ||
Identity: &collectorpb.HostID{ | ||
Hostname: hostname, | ||
}, | ||
ClientVersion: "2", | ||
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. In I wonder if this should be set as part of the package coming in or use the provided collector version? 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 |
||
} | ||
response, err := extension.client.GetSettings(ctx, request) | ||
if err != nil { | ||
extension.logger.Error("unable to get settings", zap.String("endpoint", extension.config.Endpoint), zap.Error(err)) | ||
return | ||
} | ||
switch result := response.GetResult(); result { | ||
case collectorpb.ResultCode_OK: | ||
if len(response.GetWarning()) > 0 { | ||
extension.logger.Warn("GetSettings succeed", zap.String("result", result.String()), zap.String("warning", response.GetWarning())) | ||
} | ||
var settings []map[string]any | ||
for _, item := range response.GetSettings() { | ||
setting := make(map[string]any) | ||
setting["type"] = item.GetType().Number() | ||
setting["flags"] = string(item.GetFlags()) | ||
setting["timestamp"] = item.GetTimestamp() | ||
setting["value"] = item.GetValue() | ||
setting["layer"] = string(item.GetLayer()) | ||
arguments := make(map[string]any) | ||
if value, ok := item.Arguments["BucketCapacity"]; ok { | ||
arguments["BucketCapacity"] = math.Float64frombits(binary.LittleEndian.Uint64(value)) | ||
} | ||
if value, ok := item.Arguments["BucketRate"]; ok { | ||
arguments["BucketRate"] = math.Float64frombits(binary.LittleEndian.Uint64(value)) | ||
} | ||
if value, ok := item.Arguments["TriggerRelaxedBucketCapacity"]; ok { | ||
arguments["TriggerRelaxedBucketCapacity"] = math.Float64frombits(binary.LittleEndian.Uint64(value)) | ||
} | ||
if value, ok := item.Arguments["TriggerRelaxedBucketRate"]; ok { | ||
arguments["TriggerRelaxedBucketRate"] = math.Float64frombits(binary.LittleEndian.Uint64(value)) | ||
} | ||
if value, ok := item.Arguments["TriggerStrictBucketCapacity"]; ok { | ||
arguments["TriggerStrictBucketCapacity"] = math.Float64frombits(binary.LittleEndian.Uint64(value)) | ||
} | ||
if value, ok := item.Arguments["TriggerStrictBucketRate"]; ok { | ||
arguments["TriggerStrictBucketRate"] = math.Float64frombits(binary.LittleEndian.Uint64(value)) | ||
} | ||
if value, ok := item.Arguments["MetricsFlushInterval"]; ok { | ||
arguments["MetricsFlushInterval"] = int32(binary.LittleEndian.Uint32(value)) | ||
} | ||
if value, ok := item.Arguments["MaxTransactions"]; ok { | ||
arguments["MaxTransactions"] = int32(binary.LittleEndian.Uint32(value)) | ||
} | ||
if value, ok := item.Arguments["MaxCustomMetrics"]; ok { | ||
arguments["MaxCustomMetrics"] = int32(binary.LittleEndian.Uint32(value)) | ||
} | ||
if value, ok := item.Arguments["EventsFlushInterval"]; ok { | ||
arguments["EventsFlushInterval"] = int32(binary.LittleEndian.Uint32(value)) | ||
} | ||
if value, ok := item.Arguments["ProfilingInterval"]; ok { | ||
arguments["ProfilingInterval"] = int32(binary.LittleEndian.Uint32(value)) | ||
} | ||
setting["arguments"] = arguments | ||
setting["ttl"] = item.GetTtl() | ||
settings = append(settings, setting) | ||
} | ||
if content, err := json.Marshal(settings); err != nil { | ||
extension.logger.Warn("error to marshal setting JSON[] byte from settings", zap.Error(err)) | ||
} else { | ||
if err := os.WriteFile(filename, content, 0600); err != nil { | ||
extension.logger.Error("unable to write "+filename, zap.Error(err)) | ||
} else { | ||
if len(response.GetWarning()) > 0 { | ||
extension.logger.Warn(filename + " is refreshed (soft disabled)") | ||
} else { | ||
extension.logger.Info(filename + " is refreshed") | ||
} | ||
extension.logger.Info(string(content)) | ||
} | ||
} | ||
default: | ||
extension.logger.Warn("GetSettings failed", zap.String("result", result.String()), zap.String("warning", response.GetWarning())) | ||
} | ||
} | ||
} |
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.
This does make the assumption that you're only going to be running on Unix based system, could I suggest that you use something like os.TempDir instead?
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.
For sure. We run on an Unix based system only. It doesn't hurt to use a cross platform function.
Done.