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

eBPF config provider #1481

Merged
merged 12 commits into from
Sep 2, 2024
Merged

eBPF config provider #1481

merged 12 commits into from
Sep 2, 2024

Conversation

RonFed
Copy link
Contributor

@RonFed RonFed commented Sep 1, 2024

  • Add an extension to the existing OtelEbpfSdk which allows to send configuration to a running SDK - the extended interface is called ConfigurableOtelEbpfSdk.
  • Implement a basic config provider which is generic but currently only used by the Go instrumentation.
  • Update the dependency of go.opentelemetry.io/auto
  • Add Languages method to InstrumentationConfig which returns the relevant languages specified in the CRD - this allows filtering the call to the relevant eBPF directors.

@RonFed RonFed marked this pull request as ready for review September 1, 2024 13:11
@RonFed RonFed marked this pull request as draft September 1, 2024 13:12
return nil
}

func (c *ConfigProvider[C]) Watch() <-chan C {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we pass context.Context here as well? And maybe cancel it when we want to stop watching?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure since it is ment to be a getter for the channel.
The calling code in the instrumentation looks like:

	for {
		select {
		case <-ctx.Done():
			return
		case c, ok := <-m.cp.Watch():
			if !ok {
				m.logger.Info("Configuration provider closed, configuration updates will no longer be received")
				return
			}
			err := m.applyConfig(c)
			if err != nil {
				m.logger.Error(err, "Failed to apply config")
				continue
			}
			m.currentConfig = c
		}
	}

The way to stop watching should be to call Shutdown - which is called by the instrumentation once it is closing

select {
case c.configChan <- newConfig:
return nil
case <-ctx.Done():
Copy link
Contributor

Choose a reason for hiding this comment

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

We assume the caller passed ctx which has a timeout, maybe a better design would be to get both context and time.Time or something as arguments and wrap the context in this function instead of assuming the caller did it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added timeout of 50ms for sending configuration

@RonFed RonFed marked this pull request as ready for review September 2, 2024 07:50
@RonFed RonFed merged commit 4e71626 into odigos-io:main Sep 2, 2024
22 checks passed
@RonFed RonFed deleted the ebpf_config_provider branch September 2, 2024 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants