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

[processor/tailsampling] Include componentID as prefix in metrics 'policy' #34192

Conversation

EOjeah
Copy link
Contributor

@EOjeah EOjeah commented Jul 22, 2024

Description:
Fixing a bug - This change includes the componentID as a dot prefix to the metrics policy dimension when generating metrics for the processor. The change ensures that similarly named policy's in the tail sampling processor that belong to different components also has a unique value in the policy field for the metrics.

Also includes minor refactor change to rename telemetry to telemetryBuilder where applicable (return type == NewTelemetryBuilder)

Resolves: #34099

Link to tracking Issue:

Testing:
Ran the collector locally with make run with the configuration below which uses the tail sampling processor and has metrics exposed in prometheus format. Sending sample zipkin spans to the receiver

receivers:
  zipkin:

processors:
  tail_sampling:
    policies:
      [
          {
            name: test-policy-1,
            type: always_sample
          }
      ]

  tail_sampling/custom_name:
    policies:
      [
          {
            name: test-policy-1,
            type: always_sample
          }
      ]

exporters:
  debug:

service:
  telemetry:
    logs:
    metrics:
  pipelines:
    traces:
      receivers: [zipkin]
      processors: [tail_sampling, tail_sampling/custom_name]
      exporters: [debug]

Curling the metrics endpoint shows the policy name is unique for both tail sampling processors

otelcol_processor_tail_sampling_sampling_decision_latency_bucket{policy="custom_name.test-policy-1",service_instance_id="X",service_name="otelcontribcol",service_version="0.105.0-dev",le="5000"} 1

otelcol_processor_tail_sampling_sampling_decision_latency_bucket{policy="test-policy-1",service_instance_id="X",service_name="otelcontribcol",service_version="0.105.0-dev",le="5000"} 1

Tasks

  • Confirm prefix separator as .
  • Update change log entry

This change includes the componentID as a dot prefix to the metrics
'policy' dimension when generating metrics for the processor. The change
ensures that similarly named policys in the tail sampling processor that
belong to different components also has a unique value.

Resolves: open-telemetry#34099
@EOjeah EOjeah requested a review from jpkrohling as a code owner July 22, 2024 11:10
@EOjeah EOjeah requested a review from a team July 22, 2024 11:10
@github-actions github-actions bot added the processor/tailsampling Tail sampling processor label Jul 22, 2024
Copy link
Contributor

github-actions bot commented Aug 6, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Aug 6, 2024
@EOjeah
Copy link
Contributor Author

EOjeah commented Aug 11, 2024

Not urgent but tagging @jpkrohling (currently on holiday) just so it's not closed due to inactivity

@github-actions github-actions bot removed the Stale label Aug 12, 2024
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

There seems to be a lot of unrelated changes in this PR, like the renaming of telemetry to telemetryBuilder. The actual change is quite small and localized, and reviewing it would have been otherwise straightforward.

I have not seen new tests exercising the new code path: can you please add a new test?

EOjeah added 2 commits August 21, 2024 23:32
Undo previous rename of telemtry to telemetryBuilder as it's not related to the attached issue. In order for the component ID to be passed from settings, the newTraceProcessor definition had to be chanaged to accept processor.Settings and not just the TelemetrySettings
…rib into bug/unique-policy-name-tail-sampling-processor
@EOjeah
Copy link
Contributor Author

EOjeah commented Aug 22, 2024

Reverted the change that renamed telemetry to telemetryBuilder.

In order to receive the processor settings, which contains the component.ID in the newTracesProcessor function, the newTracesProcessor function had to be updated to accept the processor.Settings and not just the processor.Settings.TelemetrySettings

A few of the existing tests use the function and provided the TelemetrySettings, updated them to provide the processor Settings instead

Settings

// Settings is passed to Create* functions in Factory.
type Settings struct {
  // ID returns the ID of the component that will be created.
  ID component.ID

  component.TelemetrySettings

  // BuildInfo can be used by components for informational purposes
  BuildInfo component.BuildInfo
}

Included a test that verifies the policy value in the metrics generated includes the ID as a dot . prefix

@EOjeah EOjeah requested a review from jpkrohling August 22, 2024 04:12
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

The actual change LGTM, just not sure about the reason for renaming the telemetry settings.

func newTracesProcessor(ctx context.Context, settings component.TelemetrySettings, nextConsumer consumer.Traces, cfg Config, opts ...Option) (processor.Traces, error) {
telemetry, err := metadata.NewTelemetryBuilder(settings)
func newTracesProcessor(ctx context.Context, set processor.Settings, nextConsumer consumer.Traces, cfg Config, opts ...Option) (processor.Traces, error) {
telemetrySettings := set.TelemetrySettings
Copy link
Member

Choose a reason for hiding this comment

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

Is there a good reason for renaming component.TelemetrySettings from settings to telemetrySettings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was only to differentiate between the new argument processor.Settings (settings) from initial argument which was processor.Settings.TelemetrySettings (telemetrySettings)

Do i change it to below instead?

func newTracesProcessor(ctx context.Context, processorSettings processor.Settings, ...) (processor.Traces, error) {
  settings := processorSettings.TelemetrySettings
  telemetry, err := metadata.NewTelemetryBuilder(settings)
  if err != nil {
    return nil, err
  }
  ...
}

Copy link
Member

Choose a reason for hiding this comment

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

No, it's okay. This is small enough to not be a problem.

EOjeah added 4 commits August 22, 2024 14:57
…rib into bug/unique-policy-name-tail-sampling-processor
…rib into bug/unique-policy-name-tail-sampling-processor
…rib into bug/unique-policy-name-tail-sampling-processor
@EOjeah
Copy link
Contributor Author

EOjeah commented Aug 23, 2024

added the changelog entry so re-request approval

@EOjeah EOjeah requested a review from jpkrohling August 23, 2024 10:41
…rib into bug/unique-policy-name-tail-sampling-processor
@jpkrohling jpkrohling changed the title Include componentID as prefix in metrics 'policy' [processor/tailsampling] Include componentID as prefix in metrics 'policy' Aug 27, 2024
@jpkrohling jpkrohling merged commit b8ee45e into open-telemetry:main Aug 27, 2024
155 of 156 checks passed
@github-actions github-actions bot added this to the next release milestone Aug 27, 2024
f7o pushed a commit to f7o/opentelemetry-collector-contrib that referenced this pull request Sep 12, 2024
…licy' (open-telemetry#34192)

**Description:**
Fixing a bug - This change includes the componentID as a dot prefix to
the metrics `policy` dimension when generating metrics for the
processor. The change ensures that similarly named policy's in the tail
sampling processor that belong to different components also has a unique
value in the `policy` field for the metrics.

Also includes minor refactor change to rename `telemetry` to
`telemetryBuilder` where applicable (return type ==
`NewTelemetryBuilder`)

Resolves: open-telemetry#34099

**Link to tracking Issue:** <Issue number if applicable>

**Testing:** 
Ran the collector locally with `make run` with the configuration below
which uses the tail sampling processor and has metrics exposed in
prometheus format. Sending sample zipkin spans to the receiver
```yaml
receivers:
  zipkin:

processors:
  tail_sampling:
    policies:
      [
          {
            name: test-policy-1,
            type: always_sample
          }
      ]

  tail_sampling/custom_name:
    policies:
      [
          {
            name: test-policy-1,
            type: always_sample
          }
      ]

exporters:
  debug:

service:
  telemetry:
    logs:
    metrics:
  pipelines:
    traces:
      receivers: [zipkin]
      processors: [tail_sampling, tail_sampling/custom_name]
      exporters: [debug]
```

Curling the metrics endpoint shows the policy name is unique for both
tail sampling processors
```bash
otelcol_processor_tail_sampling_sampling_decision_latency_bucket{policy="custom_name.test-policy-1",service_instance_id="X",service_name="otelcontribcol",service_version="0.105.0-dev",le="5000"} 1

otelcol_processor_tail_sampling_sampling_decision_latency_bucket{policy="test-policy-1",service_instance_id="X",service_name="otelcontribcol",service_version="0.105.0-dev",le="5000"} 1
```

Tasks

- [ ] Confirm prefix separator as `.` 
- [ ] Update change log entry
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
processor/tailsampling Tail sampling processor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distinguish Tail Sampling Policies in Metrics by Prefixing Processor Name
2 participants