-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Trace Based Sampling and Minimum Severity Log Filters Implementation #43811
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR implements log filtering capabilities for the Azure Monitor OpenTelemetry exporter, adding support for minimum severity level filtering and trace-based sampling. The implementation allows logs to be filtered out based on their severity level or association with unsampled traces.
Key changes:
- Added two new filtering functions:
_is_less_than_minimum_severity_leveland_should_drop_logs_for_unsampled_traces - Modified
_log_to_envelopeto returnNonefor filtered logs instead of always returning aTelemetryItem - Added comprehensive test coverage for both filtering mechanisms
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| _exporter.py | Modified log export logic to filter envelopes and handle None returns from conversion function |
| _utils.py | Added two utility functions for severity level and trace-based filtering logic |
| _constants.py | Added environment variable constants for the new filtering features |
| test_logs.py | Added comprehensive test cases for severity filtering, trace-based filtering, and combined filtering |
Comments suppressed due to low confidence (1)
sdk/monitor/azure-monitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/logs/_exporter.py:201
- The Event telemetry path (lines 189-201) lacks filtering logic that is present in Exception and Message telemetry paths. Event telemetry should also be filtered based on trace-based sampling and minimum severity level for consistency. Consider adding the same filtering checks after line 201.
elif _log_data_is_event(log_data): # Event telemetry
_set_statsbeat_custom_events_feature()
envelope.name = "Microsoft.ApplicationInsights.Event"
event_name = ""
if log_record.attributes.get(_MICROSOFT_CUSTOM_EVENT_NAME): # type: ignore
event_name = str(log_record.attributes.get(_MICROSOFT_CUSTOM_EVENT_NAME)) # type: ignore
else:
event_name = _map_body_to_message(log_record.body)
data = TelemetryEventData( # type: ignore
name=event_name,
properties=properties,
)
envelope.data = MonitorBase(base_data=data, base_type="EventData")
sdk/monitor/azure-monitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/_utils.py
Outdated
Show resolved
Hide resolved
sdk/monitor/azure-monitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/_utils.py
Outdated
Show resolved
Hide resolved
sdk/monitor/azure-monitor-opentelemetry-exporter/tests/logs/test_logs.py
Outdated
Show resolved
Hide resolved
sdk/monitor/azure-monitor-opentelemetry-exporter/tests/logs/test_logs.py
Outdated
Show resolved
Hide resolved
sdk/monitor/azure-monitor-opentelemetry-exporter/tests/logs/test_logs.py
Outdated
Show resolved
Hide resolved
|
Discussed with Hector, and let's align on using the configuration option for trace-based log sampling. We can also remove the addition of minimum-severity based log sampling for now and await the upstream changes. The relevant Node.js PR making this change is: Azure/azure-sdk-for-js#28993 |
Description
Implement open-telemetry/opentelemetry-specification#4612 to filter logs based on
min_severity_levelandtrace_based_samplingparameters.All SDK Contribution checklist:
General Guidelines and Best Practices