Add WithServiceName and WithPid as config options for instrumentation… #346
Conversation
…. Call the cleanup method of bpffs when needed
| return c | ||
| } | ||
|
|
||
| func (c instConfig) validate() error { | ||
| if c.target == nil { | ||
| return errUndefinedTarget | ||
| } | ||
| if c.serviceName == "" { | ||
| return errUndefinedServiceName |
There was a problem hiding this comment.
Should this be an error, or shouldn't it just use the resource default service name if not set?
There was a problem hiding this comment.
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.
instrumentation.go
Outdated
| // WithPID returns an [InstrumentationOption] corresponding to the executable | ||
| // used by the provided pid. | ||
| // | ||
| // If WithTarget is used, the one which is used last will take precedence. |
There was a problem hiding this comment.
WithTarget docs need to be updated to include this conflict as well.
| exePath, err := os.Readlink(exeLinkPath) | ||
| if err != nil { | ||
| log.Logger.Error(err, "Failed to read link for process exe") | ||
| exePath = "" |
There was a problem hiding this comment.
This likely shouldn't return WithTarget at this point. Maybe just return nil and ensure newInstConfig handles that value appropriately.
| @@ -45,3 +46,7 @@ func (a *Allocator) Load(ctx *context.InstrumentorContext) error { | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| func (a *Allocator) Clean(target *process.TargetDetails) error { | |||
| // | ||
| // If OTEL_SERVICE_NAME is defined it will take precedence over any value | ||
| // passed here. | ||
| func WithServiceName(serviceName string) InstrumentationOption { |
There was a problem hiding this comment.
This needs a changelog entry in an Added section.
| // used by the provided pid. | ||
| // | ||
| // If WithTarget is used, the one which is used last will take precedence. | ||
| func WithPID(pid int) InstrumentationOption { |
There was a problem hiding this comment.
This needs a changelog entry in an Added section.
Co-authored-by: Tyler Yahn <[email protected]>
|
The tests are missing. |
@pellared Where do you think would be a good place to add tests for these options? I couldn't find tests for the existing one |
Maybe the first 2 can be in the same PR since they are very similar? |
I am open to suggestions. Personally, I think both options deserve integration/e2e tests. For service name handling we could also add unit tests for |
This PR add two InstrumentationOption:
WithServiceNamesimilar toWithTarget(with the same behavior for which environment variable is possible and will take precedence).WithPIDwhich callsWithTarget.These will be useful for https://github.com/keyval-dev/odigos/blob/main/odiglet/pkg/ebpf/director.go
In addition the
Cleanupin bpffs was not called which resulted in garbage folders under /sys/fs/bpf not get deleted after the instrumentation is finished