-
Notifications
You must be signed in to change notification settings - Fork 90
Go: Add otel support #3932
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
base: main
Are you sure you want to change the base?
Go: Add otel support #3932
Conversation
291ba20
to
6165c47
Compare
65056af
to
cae60ca
Compare
cae60ca
to
79ecdeb
Compare
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.
@tjzhang-BQ thank you for the PR!
// SetSamplePercentage sets the percentage of requests to be sampled and traced. | ||
// Must be a value between 0 and 100. | ||
// This setting only affects traces, not metrics. | ||
func (o *OpenTelemetry) SetSamplePercentage(percentage int32) error { |
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.
The SetSamplePercentage
method modifies shared configuration without synchronization, which could lead to race conditions in concurrent environments. Consider adding proper synchronization:
var configMutex sync.RWMutex
func (o *OpenTelemetry) SetSamplePercentage(percentage int32) error {
configMutex.Lock()
defer configMutex.Unlock()
if !o.IsInitialized() || otelConfig == nil || otelConfig.Traces == nil {
return fmt.Errorf("OpenTelemetry config traces not initialized")
}
otelConfig.Traces.SamplePercentage = percentage
return nil
}
func (o *OpenTelemetry) GetSamplePercentage() int32 {
configMutex.RLock()
defer configMutex.RUnlock()
if !o.IsInitialized() || otelConfig == nil || otelConfig.Traces == nil {
return 0
}
return otelConfig.Traces.SamplePercentage
}
Don't forget to add the import: import "sync"
"unsafe" | ||
) | ||
|
||
// OpenTelemetryConfig represents the configuration for OpenTelemetry integration. |
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.
nit:
The code includes good documentation, but could benefit from more examples and clearer explanations of the OpenTelemetry integration, especially for users who might not be familiar with OpenTelemetry concepts. Consider adding more comprehensive documentation with examples:
// OpenTelemetryConfig represents the configuration for OpenTelemetry integration.
//
// Example usage:
//
// config := glide.OpenTelemetryConfig{
// Traces: &glide.OpenTelemetryTracesConfig{
// Endpoint: "http://localhost:4318/v1/traces",
// SamplePercentage: 10, // Sample 10% of commands
// },
// FlushIntervalMs: &interval, // interval := int64(1000)
// }
// err := glide.GetInstance().Init(config)
// if err != nil {
// log.Fatalf("Failed to initialize OpenTelemetry: %v", err)
// }
//
} | ||
Err(e) => { | ||
let error_msg = format!("Invalid traces exporter configuration: {}", e); | ||
return CString::new(error_msg) |
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.
CString
allocates memory. Where is the function to free this?
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.
under the impression that it gets freed here in the Go code that receives error messages from Rust, would this be enough?
Signed-off-by: TJ Zhang <[email protected]>
Signed-off-by: TJ Zhang <[email protected]>
Signed-off-by: TJ Zhang <[email protected]>
Signed-off-by: TJ Zhang <[email protected]>
Signed-off-by: TJ Zhang <[email protected]>
Signed-off-by: TJ Zhang <[email protected]>
Signed-off-by: TJ Zhang <[email protected]>
Signed-off-by: TJ Zhang <[email protected]>
Signed-off-by: TJ Zhang <[email protected]>
e39135a
to
a1d2334
Compare
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.
LGTM on the Go changes. I think it will be a good idea to add the config example as Alexey pointed out. I did not review FFI changes.
} | ||
|
||
if openTelemetryConfig.Traces != nil { | ||
cConfig.traces.endpoint = C.CString(openTelemetryConfig.Traces.Endpoint) |
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.
can consider explicitly freeing the memory here, to avoid a memory leak eveytime Init is called .
a simple :
defer C.free(unsafe.Pointer(cConfig.traces.endpoint))
would do.
|
||
if openTelemetryConfig.Metrics != nil { | ||
cConfig.metrics.endpoint = C.CString(openTelemetryConfig.Metrics.Endpoint) | ||
} |
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.
also at this point you can add
defer C.free(unsafe.Pointer(cConfig.metrics.endpoint))
old PR closed to re-target at main: #3834
Transaction support is pending the Go transaction to finish, to be done in a separate pr, will add the link here.
Issue link
This Pull Request is linked to issue (URL): #3531
Checklist
Before submitting the PR make sure the following are checked: