otel_access_log: support resource detectors for defining monitored resource - #47128
otel_access_log: support resource detectors for defining monitored resource#47128TAOXUY wants to merge 4 commits into
Conversation
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
37d9961 to
6558427
Compare
…source Commit Message: Add support for OpenTelemetry resource detectors in the OpenTelemetry access logger. This allows users to dynamically detect and populate resource attributes and schema URL from environment variables or custom resource detectors. Additional Description: - Added resource_detectors to OpenTelemetryAccessLogConfig proto. - Integrated ResourceProvider with otlp_log_utils to execute configured resource detectors and populate detected attributes and schema URL. - Updated gRPC and HTTP access logger implementations and caches to pass ServerFactoryContext. - Added comprehensive unit tests for config, message root initialization, gRPC logger, and HTTP logger. Risk Level: Low Testing: Added unit tests in otlp_log_utils_test, config_test, grpc_access_log_impl_test, and http_access_log_impl_test. Docs Changes: Updated proto documentation and release notes in changelogs/current.yaml. Release Notes: Added under access_loggers. Signed-off-by: Xuyang Tao <taoxuy@google.com>
6558427 to
747cbdb
Compare
|
CC @kyessenov |
kyessenov
left a comment
There was a problem hiding this comment.
Mostly minor comments. I think you need to handle merging. Can you modify an existing integration test to set some static attributes via resource detector?
| namespace OpenTelemetry { | ||
|
|
||
| namespace { | ||
| using opentelemetry::proto::collector::logs::v1::ExportLogsServiceRequest; |
There was a problem hiding this comment.
Why is this change needed?
| // Attributes detected by ``resource_detectors`` will be added to the resource attributes. | ||
| // If both ``resource_detectors`` and ``resource_attributes`` are specified, the attributes from | ||
| // ``resource_detectors`` are added first, and any attributes configured in ``resource_attributes`` | ||
| // are appended afterwards. |
There was a problem hiding this comment.
How are conflicts resolved? Are they overriding each other?
| resource_logs->set_schema_url(detected_resource.schema_url_); | ||
| } | ||
| for (const auto& [key, value] : detected_resource.attributes_) { | ||
| *resource->add_attributes() = getStringKeyValue(key, value); |
There was a problem hiding this comment.
I think this may cause conflicts with the other attribute setters? Seems like there could be duplicated keys.
| config, | ||
| const LocalInfo::LocalInfo& local_info); | ||
| Server::Configuration::ServerFactoryContext& context, | ||
| const Extensions::Tracers::OpenTelemetry::ResourceProvider& resource_provider = |
There was a problem hiding this comment.
Please use OptRef to avoid unnecessary allocations.
7614d3e to
72ee3ae
Compare
Commit Message: otel_access_log: address review comments Additional Description: - Documented attribute precedence and deduplication in proto comments. - Enforced attribute precedence order (resource_attributes > resource_detectors > built-in labels) and avoided duplicate keys in otlp_log_utils. - Updated initOtlpMessageRoot to accept OptRef<const ResourceProvider> to avoid unnecessary allocations. - Fixed [[nodiscard]] test warnings and removed unused namespace in http_access_log_impl.cc. - Added unit tests for attribute precedence and deduplication. Risk Level: Low Testing: Added unit tests and verified all tests pass. Docs Changes: Updated proto documentation. Release Notes: N/A Signed-off-by: Xuyang Tao <taoxuy@google.com>
72ee3ae to
0f4f7e8
Compare
Commit Message: test: fix undeclared variables in otlp_log_utils_test Additional Description: Use kTestZone, kTestCluster, and kTestNode constants in InitOtlpMessageRootAttributePrecedenceAndDeduplication to fix compilation failure. Risk Level: Low Testing: Fixed unit test compilation. Docs Changes: N/A Release Notes: N/A Signed-off-by: Xuyang Tao <taoxuy@google.com>
| auto* root = resource_logs->add_scope_logs(); | ||
| auto* resource = resource_logs->mutable_resource(); | ||
|
|
||
| std::vector<opentelemetry::proto::common::v1::KeyValue> attributes; |
There was a problem hiding this comment.
You could probably just use RepeatedPtrField<string>*?
… test Commit Message: otel_access_log: optimize attribute allocation and update integration test Additional Description: - Directly mutate RepeatedPtrField in initOtlpMessageRoot to avoid intermediate vector allocations. - Updated access log integration test to configure resource detectors and verify detected resource attributes. Risk Level: Low Testing: Verified unit and integration test configs. Docs Changes: N/A Release Notes: N/A Signed-off-by: Xuyang Tao <taoxuy@google.com>
Commit Message:
Add support for OpenTelemetry resource detectors in the OpenTelemetry access logger. This allows users to detect resource attributes from env.
Additional Description:
AI Assistance: This change was developed with the assistance of an AI coding agent (Jetski/Gemini). The author has reviewed, verified, tested, and takes full ownership of all submitted code.
Risk Level: Low
Testing: Added unit tests in otlp_log_utils_test, config_test, grpc_access_log_impl_test, and http_access_log_impl_test.
Docs Changes: Updated proto documentation and release notes in changelogs/current.yaml.
Release Notes: Added under access_loggers.