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

Add WithServiceName and WithPid as config options for instrumentation… #346

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
59 changes: 54 additions & 5 deletions instrumentation.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,13 @@ import (
"go.opentelemetry.io/auto/internal/pkg/process"
)

// envTargetExeKey is the key for the environment variable value pointing to the
// target binary to instrument.
const envTargetExeKey = "OTEL_GO_AUTO_TARGET_EXE"
const (
// envTargetExeKey is the key for the environment variable value pointing to the
// target binary to instrument.
envTargetExeKey = "OTEL_GO_AUTO_TARGET_EXE"
// envServiceName is the key for the envoriment variable value containing the service name.
envServiceNameKey = "OTEL_SERVICE_NAME"
)

// Instrumentation manages and controls all OpenTelemetry Go
// auto-instrumentation.
Expand All @@ -41,6 +45,8 @@ var (
// Error message returned when instrumentation is launched without a target
// binary.
errUndefinedTarget = fmt.Errorf("undefined target Go binary, consider setting the %s environment variable pointing to the target binary to instrument", envTargetExeKey)
// Error message returned when no service name is specified.
errUndefinedServiceName = fmt.Errorf("undefined service name, consider setting %s", envServiceNameKey)
)

// NewInstrumentation returns a new [Instrumentation] configured with the
Expand All @@ -57,7 +63,7 @@ func NewInstrumentation(opts ...InstrumentationOption) (*Instrumentation, error)
return nil, err
}

ctrl, err := opentelemetry.NewController(Version())
ctrl, err := opentelemetry.NewController(Version(), c.serviceName)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -106,7 +112,8 @@ type InstrumentationOption interface {
}

type instConfig struct {
target *process.TargetArgs
target *process.TargetArgs
serviceName string
}

func newInstConfig(opts []InstrumentationOption) instConfig {
Expand All @@ -122,13 +129,19 @@ func (c instConfig) applyEnv() instConfig {
if v, ok := os.LookupEnv(envTargetExeKey); ok {
c.target = &process.TargetArgs{ExePath: v}
}
if v, ok := os.LookupEnv(envServiceNameKey); ok {
c.serviceName = v
}
return c
}

func (c instConfig) validate() error {
if c.target == nil {
return errUndefinedTarget
}
if c.serviceName == "" {
return errUndefinedServiceName
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an error, or shouldn't it just use the resource default service name if not set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, we are not allowing for not setting the service name env variable. So this code preserves that.
We can change it to a default service name if not set.

}
return c.target.Validate()
}

Expand All @@ -150,3 +163,39 @@ func WithTarget(path string) InstrumentationOption {
return c
})
}

// WithServiceName returns an [InstrumentationOption] defining the name of the service running.
//
// If multiple of these options are provided to an [Instrumentation], the last
// one will be used.
//
// If OTEL_SERVICE_NAME is defined it will take precedence over any value
// passed here.
func WithServiceName(serviceName string) InstrumentationOption {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a changelog entry in an Added section.

return fnOpt(func(c instConfig) instConfig {
c.serviceName = serviceName
return c
})
}

// WithPID returns an [InstrumentationOption] corresponding to the executable
// used by the provided pid.
//
// This option conflicts with [WithTarget]. If both are used, the last one
// passed to [Instrumentation] will take precedence and be used.
//
// If multiple of these options are provided to an [Instrumentation], the last
// one will be used.
//
// If OTEL_GO_AUTO_TARGET_EXE is defined it will take precedence over any value
// passed here.
func WithPID(pid int) InstrumentationOption {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a changelog entry in an Added section.

exeLinkPath := fmt.Sprintf("/proc/%d/exe", pid)
exePath, err := os.Readlink(exeLinkPath)
if err != nil {
log.Logger.Error(err, "Failed to read exe link for process", "pid", pid)
exePath = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

This likely shouldn't return WithTarget at this point. Maybe just return nil and ensure newInstConfig handles that value appropriately.

}

return WithTarget(exePath)
}
5 changes: 5 additions & 0 deletions internal/pkg/instrumentors/allocator/allocator_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"go.opentelemetry.io/auto/internal/pkg/instrumentors/bpffs"
"go.opentelemetry.io/auto/internal/pkg/instrumentors/context"
"go.opentelemetry.io/auto/internal/pkg/log"
"go.opentelemetry.io/auto/internal/pkg/process"
)

// Allocator handles the allocation of the BPF file-system.
Expand Down Expand Up @@ -45,3 +46,7 @@ func (a *Allocator) Load(ctx *context.InstrumentorContext) error {

return nil
}

func (a *Allocator) Clean(target *process.TargetDetails) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing comment.

return bpffs.Cleanup(target)
}
15 changes: 11 additions & 4 deletions internal/pkg/instrumentors/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,11 @@ func (m *Manager) Run(ctx context.Context, target *process.TargetDetails) error
select {
case <-ctx.Done():
m.Close()
m.cleanup()
m.cleanup(target)
return ctx.Err()
case <-m.done:
log.Logger.V(0).Info("shutting down all instrumentors due to signal")
m.cleanup()
m.cleanup(target)
return nil
case e := <-m.incomingEvents:
m.otelController.Trace(e)
Expand Down Expand Up @@ -91,7 +91,7 @@ func (m *Manager) load(target *process.TargetDetails) error {
err := i.Load(ctx)
if err != nil {
log.Logger.Error(err, "error while loading instrumentors, cleaning up", "name", name)
m.cleanup()
m.cleanup(target)
return err
}
}
Expand All @@ -100,11 +100,18 @@ func (m *Manager) load(target *process.TargetDetails) error {
return nil
}

func (m *Manager) cleanup() {
func (m *Manager) cleanup(target *process.TargetDetails) {
close(m.incomingEvents)
for _, i := range m.instrumentors {
i.Close()
}

log.Logger.V(0).Info("Cleaning bpffs")
err := m.allocator.Clean(target)

if err != nil {
log.Logger.Error(err, "Failed to clean bpffs")
}
}

// Close closes m.
Expand Down
12 changes: 1 addition & 11 deletions internal/pkg/opentelemetry/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ package opentelemetry
import (
"context"
"fmt"
"os"
"runtime"
"strings"
"time"
Expand All @@ -35,10 +34,6 @@ import (
"go.opentelemetry.io/otel/trace"
)

const (
otelServiceNameEnvVar = "OTEL_SERVICE_NAME"
)

// Information about the runtime environment for inclusion in User-Agent, e.g. "go/1.18.2 (linux/amd64)".
var runtimeInfo = fmt.Sprintf("%s (%s/%s)", strings.Replace(runtime.Version(), "go", "go/", 1), runtime.GOOS, runtime.GOARCH)

Expand Down Expand Up @@ -89,12 +84,7 @@ func (c *Controller) convertTime(t int64) time.Time {
}

// NewController returns a new initialized [Controller].
func NewController(version string) (*Controller, error) {
serviceName, exists := os.LookupEnv(otelServiceNameEnvVar)
if !exists {
return nil, fmt.Errorf("%s env var must be set", otelServiceNameEnvVar)
}

func NewController(version string, serviceName string) (*Controller, error) {
ctx := context.Background()
res, err := resource.New(ctx,
resource.WithAttributes(
Expand Down
3 changes: 1 addition & 2 deletions internal/pkg/process/discover.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ package process

import (
"io"
"io/ioutil"
"os"
"path"
"strconv"
Expand Down Expand Up @@ -97,7 +96,7 @@ func (a *Analyzer) findProcessID(target *TargetArgs) (int, error) {
exeName, err := os.Readlink(path.Join("/proc", dname, "exe"))
if err != nil {
// Read link may fail if target process runs not as root
cmdLine, err := ioutil.ReadFile(path.Join("/proc", dname, "cmdline"))
cmdLine, err := os.ReadFile(path.Join("/proc", dname, "cmdline"))
if err != nil {
return 0, err
}
Expand Down
Loading