Skip to content

Commit

Permalink
code review changes
Browse files Browse the repository at this point in the history
  • Loading branch information
atoulme committed Jul 24, 2023
1 parent a993d85 commit 0147fd5
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 9 deletions.
6 changes: 4 additions & 2 deletions .chloggen/metric-server-hostproc.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: bug_fix
change_type: breaking

# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
component: service/proctelemetry
Expand All @@ -13,4 +13,6 @@ issues: [7434]
# (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: This is an API change only, allowing users to set a value to override the environment variable HOST_PROC.
subtext: |
This is an API change only, allowing users to set a value to override the environment variable HOST_PROC.
This change affects the proctelemetry.RegisterProcessMetrics function, adding a vararg argument to pass options.
31 changes: 28 additions & 3 deletions service/internal/proctelemetry/process_telemetry.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,34 @@ type processMetrics struct {
ms *runtime.MemStats
}

type RegisterOption interface {
apply(*registerOption)
}

type registerOption struct {
hostProc string
}

type registerOptionFunc func(*registerOption)

func (fn registerOptionFunc) apply(set *registerOption) {
fn(set)
}

// WithHostProc overrides the /proc folder on Linux used by process telemetry.
func WithHostProc(hostProc string) RegisterOption {
return registerOptionFunc(func(uo *registerOption) {
uo.hostProc = hostProc
})
}

// RegisterProcessMetrics creates a new set of processMetrics (mem, cpu) that can be used to measure
// basic information about this process.
func RegisterProcessMetrics(ocRegistry *metric.Registry, mp otelmetric.MeterProvider, useOtel bool, ballastSizeBytes uint64, hostProc string) error {
func RegisterProcessMetrics(ocRegistry *metric.Registry, mp otelmetric.MeterProvider, useOtel bool, ballastSizeBytes uint64, opts ...RegisterOption) error {
set := registerOption{}
for _, opt := range opts {
opt.apply(&set)
}
var err error
pm := &processMetrics{
startTimeUnixNano: time.Now().UnixNano(),
Expand All @@ -62,8 +87,8 @@ func RegisterProcessMetrics(ocRegistry *metric.Registry, mp otelmetric.MeterProv
}

ctx := context.Background()
if hostProc != "" {
ctx = context.WithValue(ctx, common.EnvKey, common.EnvMap{common.HostProcEnvKey: hostProc})
if set.hostProc != "" {
ctx = context.WithValue(ctx, common.EnvKey, common.EnvMap{common.HostProcEnvKey: set.hostProc})
}
pm.context = ctx
pm.proc, err = process.NewProcessWithContext(pm.context, int32(os.Getpid()))
Expand Down
6 changes: 3 additions & 3 deletions service/internal/proctelemetry/process_telemetry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func fetchPrometheusMetrics(handler http.Handler) (map[string]*io_prometheus_cli
func TestOtelProcessTelemetry(t *testing.T) {
tel := setupTelemetry(t)

require.NoError(t, RegisterProcessMetrics(nil, tel.MeterProvider, true, 0, ""))
require.NoError(t, RegisterProcessMetrics(nil, tel.MeterProvider, true, 0))

mp, err := fetchPrometheusMetrics(tel.promHandler)
require.NoError(t, err)
Expand Down Expand Up @@ -137,7 +137,7 @@ func TestOtelProcessTelemetry(t *testing.T) {
func TestOCProcessTelemetry(t *testing.T) {
ocRegistry := metric.NewRegistry()

require.NoError(t, RegisterProcessMetrics(ocRegistry, noop.NewMeterProvider(), false, 0, ""))
require.NoError(t, RegisterProcessMetrics(ocRegistry, noop.NewMeterProvider(), false, 0))

// Check that the metrics are actually filled.
time.Sleep(200 * time.Millisecond)
Expand Down Expand Up @@ -174,7 +174,7 @@ func TestProcessTelemetryFailToRegister(t *testing.T) {
ocRegistry := metric.NewRegistry()
_, err := ocRegistry.AddFloat64Gauge(metricName)
require.NoError(t, err)
assert.Error(t, RegisterProcessMetrics(ocRegistry, noop.NewMeterProvider(), false, 0, ""))
assert.Error(t, RegisterProcessMetrics(ocRegistry, noop.NewMeterProvider(), false, 0))
})
}
}
Expand Down
2 changes: 1 addition & 1 deletion service/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ func (srv *Service) initExtensionsAndPipeline(ctx context.Context, set Settings,

if cfg.Telemetry.Metrics.Level != configtelemetry.LevelNone && cfg.Telemetry.Metrics.Address != "" {
// The process telemetry initialization requires the ballast size, which is available after the extensions are initialized.
if err = proctelemetry.RegisterProcessMetrics(srv.telemetryInitializer.ocRegistry, srv.telemetryInitializer.mp, obsreportconfig.UseOtelForInternalMetricsfeatureGate.IsEnabled(), getBallastSize(srv.host), ""); err != nil {
if err = proctelemetry.RegisterProcessMetrics(srv.telemetryInitializer.ocRegistry, srv.telemetryInitializer.mp, obsreportconfig.UseOtelForInternalMetricsfeatureGate.IsEnabled(), getBallastSize(srv.host)); err != nil {
return fmt.Errorf("failed to register process metrics: %w", err)
}
}
Expand Down

0 comments on commit 0147fd5

Please sign in to comment.