-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[processor/tailsampling] Include componentID as prefix in metrics 'policy' #34192
Conversation
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
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Not urgent but tagging @jpkrohling (currently on holiday) just so it's not closed due to inactivity |
There was a problem hiding this 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?
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
Reverted the change that renamed telemetry to telemetryBuilder. In order to receive the processor settings, which contains the 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 |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
}
...
}
There was a problem hiding this comment.
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.
…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
added the changelog entry so re-request approval |
…rib into bug/unique-policy-name-tail-sampling-processor
…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
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 thepolicy
field for the metrics.Also includes minor refactor change to rename
telemetry
totelemetryBuilder
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 receiverCurling the metrics endpoint shows the policy name is unique for both tail sampling processors
Tasks
.