zipkin: add custom headers support for collector requests #40863
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change introduces the collector_request_headers field to ZipkinConfig, allowing users to add custom headers to HTTP requests sent to the Zipkin collector. This enables authentication, authorization, and collector-specific routing capabilities. The feature includes: - New collector_request_headers field in ZipkinConfig proto - CollectorInfo struct updated to store header key-value pairs - ReporterImpl modified to include headers in HTTP requests - Comprehensive test coverage for all components - Documentation updates in tracing overview and FAQ This is different from custom tags which add metadata to spans; this feature adds HTTP headers to collector API requests. Signed-off-by: Prashanth Josyula <[email protected]>
This change introduces the collector_request_headers field to ZipkinConfig, allowing users to add custom headers to HTTP requests sent to the Zipkin collector. This enables authentication, authorization, and collector-specific routing capabilities. The feature includes: - New collector_request_headers field in ZipkinConfig proto - CollectorInfo struct updated to store header key-value pairs - ReporterImpl modified to include headers in HTTP requests - Comprehensive test coverage for all components - Documentation updates in tracing overview and FAQ This is different from custom tags which add metadata to spans; this feature adds HTTP headers to collector API requests. Signed-off-by: Prashanth Josyula <[email protected]>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Contributor
Author
|
/coverage |
|
Coverage for this Pull Request will be rendered here: https://storage.googleapis.com/envoy-cncf-pr/40863/coverage/index.html For comparison, current coverage on https://storage.googleapis.com/envoy-cncf-postsubmit/main/coverage/index.html The coverage results are (re-)rendered each time the CI |
…onfig, allowing users to add custom headers to HTTP requests sent to the Zipkin collector. This enables authentication, authorization, and collector-specific routing capabilities. The feature includes: - New collector_request_headers field in ZipkinConfig proto - CollectorInfo struct updated to store header key-value pairs - ReporterImpl modified to include headers in HTTP requests - Comprehensive test coverage for all components - Documentation updates in tracing overview and FAQ This is different from custom tags which add metadata to spans; this feature adds HTTP headers to collector API requests. Signed-off-by: Prashanth Josyula <[email protected]>
Contributor
Author
|
/coverage |
|
Coverage for this Pull Request will be rendered here: https://storage.googleapis.com/envoy-cncf-pr/40863/coverage/index.html For comparison, current coverage on https://storage.googleapis.com/envoy-cncf-postsubmit/main/coverage/index.html The coverage results are (re-)rendered each time the CI |
Contributor
Author
|
In the pre-check an unrelated test is failing, may be a re-run will fix it. |
Contributor
|
/assign @wbpcode for Zipkin codeowner approval |
Contributor
|
/retest |
wbpcode
reviewed
Aug 28, 2025
Comment on lines
111
to
116
|
|
||
| // Additional HTTP headers to include in requests sent to the Zipkin collector. | ||
| // These headers will be added to all HTTP requests sent to the collector endpoint. | ||
| // This can be useful for authentication, authorization, or collector-specific routing. | ||
| // Note: This is different from custom tags which add metadata to individual spans. | ||
| repeated core.v3.HeaderValue collector_request_headers = 9; |
Member
There was a problem hiding this comment.
I personally think we should use the config.core.v3.HttpService here and then we can deprecate the collector_cluster , collector_endpoint, collector_hostname at next version.
cc @envoyproxy/api-shepherds
Contributor
Author
There was a problem hiding this comment.
Got it, seems fair enough, but don’t we still require collector_cluster, we can’t deprecate that right?
Contributor
Author
There was a problem hiding this comment.
Ahh I see cluster in the HttpUri
Contributor
Author
There was a problem hiding this comment.
@wbpcode I made the changes can you take a look again?
…onfig, allowing users to add custom headers to HTTP requests sent to the Zipkin collector. This enables authentication, authorization, and collector-specific routing capabilities. The feature includes: - New collector_request_headers field in ZipkinConfig proto - CollectorInfo struct updated to store header key-value pairs - ReporterImpl modified to include headers in HTTP requests - Comprehensive test coverage for all components - Documentation updates in tracing overview and FAQ This is different from custom tags which add metadata to spans; this feature adds HTTP headers to collector API requests. Signed-off-by: Prashanth Josyula <[email protected]>
…onfig, allowing users to add custom headers to HTTP requests sent to the Zipkin collector. This enables authentication, authorization, and collector-specific routing capabilities. The feature includes: - New collector_request_headers field in ZipkinConfig proto - CollectorInfo struct updated to store header key-value pairs - ReporterImpl modified to include headers in HTTP requests - Comprehensive test coverage for all components - Documentation updates in tracing overview and FAQ This is different from custom tags which add metadata to spans; this feature adds HTTP headers to collector API requests. Signed-off-by: Prashanth Josyula <[email protected]>
Signed-off-by: Prashanth Josyula <[email protected]>
…onfig, allowing users to add custom headers to HTTP requests sent to the Zipkin collector. This enables authentication, authorization, and collector-specific routing capabilities. The feature includes: - New collector_request_headers field in ZipkinConfig proto - CollectorInfo struct updated to store header key-value pairs - ReporterImpl modified to include headers in HTTP requests - Comprehensive test coverage for all components - Documentation updates in tracing overview and FAQ This is different from custom tags which add metadata to spans; this feature adds HTTP headers to collector API requests. Signed-off-by: Prashanth Josyula <[email protected]>
…onfig, allowing users to add custom headers to HTTP requests sent to the Zipkin collector. This enables authentication, authorization, and collector-specific routing capabilities. The feature includes: - New collector_request_headers field in ZipkinConfig proto - CollectorInfo struct updated to store header key-value pairs - ReporterImpl modified to include headers in HTTP requests - Comprehensive test coverage for all components - Documentation updates in tracing overview and FAQ This is different from custom tags which add metadata to spans; this feature adds HTTP headers to collector API requests. Signed-off-by: Prashanth Josyula <[email protected]>
…onfig, allowing users to add custom headers to HTTP requests sent to the Zipkin collector. This enables authentication, authorization, and collector-specific routing capabilities. The feature includes: - New collector_request_headers field in ZipkinConfig proto - CollectorInfo struct updated to store header key-value pairs - ReporterImpl modified to include headers in HTTP requests - Comprehensive test coverage for all components - Documentation updates in tracing overview and FAQ This is different from custom tags which add metadata to spans; this feature adds HTTP headers to collector API requests. Signed-off-by: Prashanth Josyula <[email protected]>
…onfig, allowing users to add custom headers to HTTP requests sent to the Zipkin collector. This enables authentication, authorization, and collector-specific routing capabilities. The feature includes: - New collector_request_headers field in ZipkinConfig proto - CollectorInfo struct updated to store header key-value pairs - ReporterImpl modified to include headers in HTTP requests - Comprehensive test coverage for all components - Documentation updates in tracing overview and FAQ This is different from custom tags which add metadata to spans; this feature adds HTTP headers to collector API requests. Signed-off-by: Prashanth Josyula <[email protected]>
Contributor
Author
|
/retry-azp |
Contributor
Author
|
/retest |
…onfig, allowing users to add custom headers to HTTP requests sent to the Zipkin collector. This enables authentication, authorization, and collector-specific routing capabilities. The feature includes: - New collector_request_headers field in ZipkinConfig proto - CollectorInfo struct updated to store header key-value pairs - ReporterImpl modified to include headers in HTTP requests - Comprehensive test coverage for all components - Documentation updates in tracing overview and FAQ This is different from custom tags which add metadata to spans; this feature adds HTTP headers to collector API requests. Signed-off-by: Prashanth Josyula <[email protected]>
wbpcode
reviewed
Sep 2, 2025
Member
wbpcode
left a comment
wbpcode
left a comment
There was a problem hiding this comment.
Thanks for the update. LGTM overall but some comment to the doc and the code. Thanks again!!!
/wait
Comment on lines
113
to
128
| The Zipkin tracer supports advanced configuration using the | ||
| :ref:`collector_service <envoy_v3_api_field_config.trace.v3.ZipkinConfig.collector_service>` | ||
| configuration with HttpService. This provides two key capabilities: | ||
|
|
||
| 1. **Custom Headers**: Add HTTP headers to collector requests for custom metadata, service identification, | ||
| or collector-specific routing (e.g., API tokens, service identifiers). | ||
|
|
||
| 2. **Full URI Parsing**: The ``uri`` field supports both path-only (``/api/v2/spans``) and full URI formats | ||
| (``https://zipkin-collector.example.com/api/v2/spans``). When using full URIs, Envoy automatically | ||
| extracts the hostname and path components - the hostname is used for the HTTP ``Host`` header, | ||
| and the path is used for the request path. Path-only URIs fallback to using the cluster name as hostname. | ||
|
|
||
| When ``collector_service`` is specified, it takes precedence over the legacy configuration fields | ||
| (``collector_cluster``, ``collector_endpoint``, ``collector_hostname``), which will be deprecated | ||
| in a future release. Legacy configuration does not support custom headers or URI parsing. | ||
| This feature is different from custom tags, which add metadata to individual spans within the trace data. |
Member
There was a problem hiding this comment.
This is unnecessary to be here. This document is mainly used to tell how different tracer propagate the context.
Contributor
Author
There was a problem hiding this comment.
Addressed the comment!
Comment on lines
53
to
65
| **Legacy Configuration (Deprecated):** | ||
|
|
||
| The legacy configuration fields (``collector_cluster``, ``collector_endpoint``, ``collector_hostname``) | ||
| will be deprecated in a future release. These fields do not support custom headers or URI parsing. | ||
| When both configurations are present, ``collector_service`` takes precedence. | ||
|
|
||
| **Migration Recommendation:** | ||
|
|
||
| Users should migrate to ``collector_service`` configuration to take advantage of custom headers | ||
| and URI parsing capabilities, and to prepare for the future deprecation of legacy fields. | ||
|
|
||
| This feature is different from custom tags, which add metadata to individual spans within the trace data. | ||
| Custom headers are added to the HTTP requests sent to the Zipkin backend for purposes like service identification. |
Member
There was a problem hiding this comment.
These are unnecessary. All deprecated fields are recommended to migrate and needn't additional description.
Contributor
Author
There was a problem hiding this comment.
Addressed the comment!
Comment on lines
16
to
51
| **Full URI Support with Automatic Parsing:** | ||
|
|
||
| The ``uri`` field supports both path-only and full URI formats: | ||
|
|
||
| .. code-block:: yaml | ||
| tracing: | ||
| provider: | ||
| name: envoy.tracers.zipkin | ||
| typed_config: | ||
| "@type": type.googleapis.com/envoy.config.trace.v3.ZipkinConfig | ||
| collector_service: | ||
| http_uri: | ||
| # Full URI format - hostname and path are extracted automatically | ||
| uri: "https://zipkin-collector.example.com/api/v2/spans" | ||
| cluster: zipkin | ||
| timeout: 5s | ||
| request_headers_to_add: | ||
| - header: | ||
| key: "X-Custom-Token" | ||
| value: "your-custom-token" | ||
| - header: | ||
| key: "X-Service-ID" | ||
| value: "your-service-id" | ||
| **URI Parsing Behavior:** | ||
|
|
||
| * **Full URI**: ``"https://zipkin-collector.example.com/api/v2/spans"`` | ||
|
|
||
| * **Hostname**: ``zipkin-collector.example.com`` (sets HTTP ``Host`` header) | ||
| * **Path**: ``/api/v2/spans`` (sets HTTP request path) | ||
|
|
||
| * **Path only**: ``"/api/v2/spans"`` | ||
|
|
||
| * **Hostname**: Uses cluster name as fallback | ||
| * **Path**: ``/api/v2/spans`` |
Member
There was a problem hiding this comment.
Please move this to the zipkin proto's comment directly. It's easier for users to read/find.
Contributor
Author
There was a problem hiding this comment.
Addressed the comment!
Comment on lines
75
to
81
| // Validate required HttpService fields | ||
| if (http_uri.cluster().empty()) { | ||
| throw EnvoyException("collector_service.http_uri.cluster must be specified"); | ||
| } | ||
| if (http_uri.uri().empty()) { | ||
| throw EnvoyException("collector_service.http_uri.uri must be specified"); | ||
| } |
Member
There was a problem hiding this comment.
there are unnecessary because the PGV will validate it.
Contributor
Author
There was a problem hiding this comment.
Addressed the comment!
Comment on lines
122
to
123
| // Whether to use legacy configuration (true) or HttpService (false) | ||
| bool use_legacy_config_{true}; |
Contributor
Author
There was a problem hiding this comment.
Addressed the comment!
Comment on lines
246
to
252
| // Add custom headers only for HttpService configuration | ||
| if (!collector_.use_legacy_config_) { | ||
| for (const auto& header : collector_.request_headers_) { | ||
| // Replace any existing header with the configured value | ||
| message->headers().setCopy(Http::LowerCaseString(header.first), header.second); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
the if check here make no sense. You can iterate the request_headers_ directly. It will be empty if legacy configuration are used.
Contributor
Author
There was a problem hiding this comment.
Addressed the comment!
| // Additional custom headers to include in requests to the Zipkin collector. | ||
| // Only available when using HttpService configuration via request_headers_to_add. | ||
| // Legacy configuration does not support custom headers. | ||
| std::vector<std::pair<std::string, std::string>> request_headers_; |
Member
There was a problem hiding this comment.
Store Http::LowercaseString here directly to avoid convert it to Http::LowerCaseString at every flush.
Contributor
Author
There was a problem hiding this comment.
Addressed the comment!
This change introduces the collector_request_headers field to ZipkinConfig, allowing users to add custom headers to HTTP requests sent to the Zipkin collector. This enables authentication, authorization, and collector-specific routing capabilities. The feature includes: - New collector_request_headers field in ZipkinConfig proto - CollectorInfo struct updated to store header key-value pairs - ReporterImpl modified to include headers in HTTP requests - Comprehensive test coverage for all components - Documentation updates in tracing overview and FAQ This is different from custom tags which add metadata to spans; this feature adds HTTP headers to collector API requests. Signed-off-by: Prashanth Josyula <[email protected]>
This change introduces the collector_request_headers field to ZipkinConfig, allowing users to add custom headers to HTTP requests sent to the Zipkin collector. This enables authentication, authorization, and collector-specific routing capabilities. The feature includes: - New collector_request_headers field in ZipkinConfig proto - CollectorInfo struct updated to store header key-value pairs - ReporterImpl modified to include headers in HTTP requests - Comprehensive test coverage for all components - Documentation updates in tracing overview and FAQ This is different from custom tags which add metadata to spans; this feature adds HTTP headers to collector API requests. Signed-off-by: Prashanth Josyula <[email protected]>
This change introduces the collector_request_headers field to ZipkinConfig, allowing users to add custom headers to HTTP requests sent to the Zipkin collector. This enables authentication, authorization, and collector-specific routing capabilities. The feature includes: - New collector_request_headers field in ZipkinConfig proto - CollectorInfo struct updated to store header key-value pairs - ReporterImpl modified to include headers in HTTP requests - Comprehensive test coverage for all components - Documentation updates in tracing overview and FAQ This is different from custom tags which add metadata to spans; this feature adds HTTP headers to collector API requests. Signed-off-by: Prashanth Josyula <[email protected]>
Contributor
Author
|
@wbpcode I resolved all the comments given by you, can you please take a look and review/approve the PR. |
wbpcode
reviewed
Sep 5, 2025
Member
wbpcode
left a comment
wbpcode
left a comment
There was a problem hiding this comment.
Thanks for the update. Almost there. And some more comments are added.
Comment on lines
1
to
140
| #include <string> | ||
| #include <utility> | ||
| #include <vector> | ||
|
|
||
| #include "envoy/config/core/v3/http_service.pb.h" | ||
| #include "envoy/http/header_map.h" | ||
|
|
||
| #include "source/extensions/tracers/zipkin/zipkin_tracer_impl.h" | ||
|
|
||
| #include "gtest/gtest.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Extensions { | ||
| namespace Tracers { | ||
| namespace Zipkin { | ||
| namespace { | ||
|
|
||
| TEST(CollectorInfoTest, DefaultConstruction) { | ||
| CollectorInfo collector_info; | ||
|
|
||
| // Default values should be set correctly | ||
| EXPECT_TRUE(collector_info.endpoint_.empty()); | ||
| EXPECT_TRUE(collector_info.hostname_.empty()); | ||
| EXPECT_EQ(collector_info.version_, envoy::config::trace::v3::ZipkinConfig::HTTP_JSON); | ||
| EXPECT_TRUE(collector_info.shared_span_context_); | ||
| EXPECT_TRUE(collector_info.request_headers_.empty()); | ||
|
|
||
| // New fields should have default values | ||
| EXPECT_FALSE(collector_info.http_service_.has_value()); | ||
| } | ||
|
|
||
| TEST(CollectorInfoTest, CustomHeadersAssignment) { | ||
| CollectorInfo collector_info; | ||
|
|
||
| // Add custom headers | ||
| collector_info.request_headers_.emplace_back(Http::LowerCaseString("Authorization"), | ||
| "Bearer token123"); | ||
| collector_info.request_headers_.emplace_back(Http::LowerCaseString("X-Custom-Header"), | ||
| "custom-value"); | ||
| collector_info.request_headers_.emplace_back(Http::LowerCaseString("X-API-Key"), "api-key-123"); | ||
|
|
||
| // Verify headers were set correctly | ||
| EXPECT_EQ(collector_info.request_headers_.size(), 3); | ||
| EXPECT_EQ(collector_info.request_headers_[0].first.get(), "authorization"); | ||
| EXPECT_EQ(collector_info.request_headers_[0].second, "Bearer token123"); | ||
| EXPECT_EQ(collector_info.request_headers_[1].first.get(), "x-custom-header"); | ||
| EXPECT_EQ(collector_info.request_headers_[1].second, "custom-value"); | ||
| EXPECT_EQ(collector_info.request_headers_[2].first.get(), "x-api-key"); | ||
| EXPECT_EQ(collector_info.request_headers_[2].second, "api-key-123"); | ||
| } | ||
|
|
||
| TEST(CollectorInfoTest, EmptyCustomHeaders) { | ||
| CollectorInfo collector_info; | ||
|
|
||
| // Explicitly clear headers | ||
| collector_info.request_headers_.clear(); | ||
|
|
||
| // Verify headers are empty | ||
| EXPECT_TRUE(collector_info.request_headers_.empty()); | ||
| EXPECT_EQ(collector_info.request_headers_.size(), 0); | ||
| } | ||
|
|
||
| TEST(CollectorInfoTest, CustomHeadersWithCompleteConfiguration) { | ||
| CollectorInfo collector_info; | ||
|
|
||
| // Set all fields including custom headers | ||
| collector_info.endpoint_ = "/api/v2/spans"; | ||
| collector_info.hostname_ = "zipkin.example.com"; | ||
| collector_info.version_ = envoy::config::trace::v3::ZipkinConfig::HTTP_PROTO; | ||
| collector_info.shared_span_context_ = false; | ||
| collector_info.request_headers_.emplace_back(Http::LowerCaseString("Content-Type"), | ||
| "application/x-protobuf"); | ||
| collector_info.request_headers_.emplace_back(Http::LowerCaseString("Authorization"), | ||
| "Bearer secret-token"); | ||
| collector_info.request_headers_.emplace_back(Http::LowerCaseString("X-Zipkin-Trace"), "enabled"); | ||
|
|
||
| // Verify all fields are set correctly | ||
| EXPECT_EQ(collector_info.endpoint_, "/api/v2/spans"); | ||
| EXPECT_EQ(collector_info.hostname_, "zipkin.example.com"); | ||
| EXPECT_EQ(collector_info.version_, envoy::config::trace::v3::ZipkinConfig::HTTP_PROTO); | ||
| EXPECT_FALSE(collector_info.shared_span_context_); | ||
| EXPECT_EQ(collector_info.request_headers_.size(), 3); | ||
|
|
||
| // Verify specific headers | ||
| EXPECT_EQ(collector_info.request_headers_[0].first.get(), "content-type"); | ||
| EXPECT_EQ(collector_info.request_headers_[0].second, "application/x-protobuf"); | ||
| EXPECT_EQ(collector_info.request_headers_[1].first.get(), "authorization"); | ||
| EXPECT_EQ(collector_info.request_headers_[1].second, "Bearer secret-token"); | ||
| EXPECT_EQ(collector_info.request_headers_[2].first.get(), "x-zipkin-trace"); | ||
| EXPECT_EQ(collector_info.request_headers_[2].second, "enabled"); | ||
| } | ||
|
|
||
| TEST(CollectorInfoTest, SingleCustomHeader) { | ||
| CollectorInfo collector_info; | ||
|
|
||
| // Add single custom header | ||
| collector_info.request_headers_.emplace_back(Http::LowerCaseString("X-Single-Header"), | ||
| "single-value"); | ||
|
|
||
| // Verify single header was set correctly | ||
| EXPECT_EQ(collector_info.request_headers_.size(), 1); | ||
| EXPECT_EQ(collector_info.request_headers_[0].first.get(), "x-single-header"); | ||
| EXPECT_EQ(collector_info.request_headers_[0].second, "single-value"); | ||
| } | ||
|
|
||
| TEST(CollectorInfoTest, LegacyConfigurationMode) { | ||
| CollectorInfo collector_info; | ||
|
|
||
| // Set up legacy configuration | ||
| collector_info.endpoint_ = "/api/v2/spans"; | ||
| collector_info.hostname_ = "zipkin.legacy.com"; | ||
|
|
||
| // Verify legacy configuration | ||
| EXPECT_EQ(collector_info.endpoint_, "/api/v2/spans"); | ||
| EXPECT_EQ(collector_info.hostname_, "zipkin.legacy.com"); | ||
| EXPECT_FALSE(collector_info.http_service_.has_value()); | ||
| } | ||
|
|
||
| TEST(CollectorInfoTest, HttpServiceConfigurationMode) { | ||
| CollectorInfo collector_info; | ||
|
|
||
| // Create mock HttpService configuration | ||
| envoy::config::core::v3::HttpService http_service; | ||
| auto* http_uri = http_service.mutable_http_uri(); | ||
| http_uri->set_uri("/api/v2/spans"); | ||
| http_uri->set_cluster("zipkin_collector"); | ||
| http_uri->mutable_timeout()->set_seconds(5); | ||
|
|
||
| // Set up HttpService configuration | ||
| collector_info.http_service_ = http_service; | ||
| collector_info.endpoint_ = "/api/v2/spans"; // Should be populated from HttpService | ||
| collector_info.hostname_ = "zipkin_collector"; // Should be populated from cluster name | ||
|
|
||
| // Verify HttpService configuration | ||
| EXPECT_TRUE(collector_info.http_service_.has_value()); | ||
| EXPECT_EQ(collector_info.http_service_->http_uri().uri(), "/api/v2/spans"); | ||
| EXPECT_EQ(collector_info.http_service_->http_uri().cluster(), "zipkin_collector"); | ||
| EXPECT_EQ(collector_info.endpoint_, "/api/v2/spans"); // Should match HttpService URI | ||
| EXPECT_EQ(collector_info.hostname_, "zipkin_collector"); // Should match cluster name | ||
| } |
Member
There was a problem hiding this comment.
IMO, all these tests could be skipped anyway because the it's more like to test the basic feature of assignment to string or vector and check the string or vector is correctly assigned? We needn't to test that I think?
Contributor
Author
There was a problem hiding this comment.
I removed all these tests!
| // | ||
| // * ``http_uri.cluster`` - Must be specified and non-empty | ||
| // * ``http_uri.uri`` - Must be specified and non-empty | ||
| // * ``http_uri.timeout`` - Must be specified |
Member
There was a problem hiding this comment.
timeout should be optional and you could provide a default value.
Contributor
Author
There was a problem hiding this comment.
Its optional, I corrected the comment!
|
|
||
| if (!parsed_hostname.empty()) { | ||
| // Use the hostname from the URI | ||
| hostname_ = parsed_hostname; |
Member
There was a problem hiding this comment.
is the hostname_ still make sense here because we have stored it in the CollectorInfo?
Contributor
Author
There was a problem hiding this comment.
The small memory overhead of storing the hostname twice is worth the clean architecture and stable public API. The duplication is intentional and serves different purposes (configuration vs. operation).
Member
There was a problem hiding this comment.
I have different opinion but anyway, this is a nit.
Comment on lines
1071
to
1138
| TEST_F(ZipkinDriverTest, DriverWithHttpServiceCustomHeaders) { | ||
| cm_.initializeClusters({"fake_cluster"}, {}); | ||
|
|
||
| const std::string yaml_string = R"EOF( | ||
| collector_cluster: fake_cluster | ||
| collector_endpoint: /api/v2/spans | ||
| collector_service: | ||
| http_uri: | ||
| uri: "https://zipkin-collector.example.com/api/v2/spans" | ||
| cluster: fake_cluster | ||
| timeout: 5s | ||
| request_headers_to_add: | ||
| - header: | ||
| key: "Authorization" | ||
| value: "Bearer token123" | ||
| - header: | ||
| key: "X-Custom-Header" | ||
| value: "custom-value" | ||
| - header: | ||
| key: "X-API-Key" | ||
| value: "api-key-123" | ||
| collector_endpoint_version: HTTP_JSON | ||
| )EOF"; | ||
|
|
||
| envoy::config::trace::v3::ZipkinConfig zipkin_config; | ||
| TestUtility::loadFromYaml(yaml_string, zipkin_config); | ||
|
|
||
| setup(zipkin_config, true); | ||
| EXPECT_NE(nullptr, driver_); | ||
| } | ||
|
|
||
| TEST_F(ZipkinDriverTest, DriverWithHttpServiceEmptyHeaders) { | ||
| cm_.initializeClusters({"fake_cluster"}, {}); | ||
|
|
||
| const std::string yaml_string = R"EOF( | ||
| collector_cluster: fake_cluster | ||
| collector_endpoint: /api/v2/spans | ||
| collector_service: | ||
| http_uri: | ||
| uri: "https://zipkin-collector.example.com/api/v2/spans" | ||
| cluster: fake_cluster | ||
| timeout: 5s | ||
| request_headers_to_add: [] | ||
| collector_endpoint_version: HTTP_JSON | ||
| )EOF"; | ||
|
|
||
| envoy::config::trace::v3::ZipkinConfig zipkin_config; | ||
| TestUtility::loadFromYaml(yaml_string, zipkin_config); | ||
|
|
||
| setup(zipkin_config, true); | ||
| EXPECT_NE(nullptr, driver_); | ||
| } | ||
|
|
||
| TEST_F(ZipkinDriverTest, DriverWithNoCustomHeaders) { | ||
| cm_.initializeClusters({"fake_cluster"}, {}); | ||
|
|
||
| const std::string yaml_string = R"EOF( | ||
| collector_cluster: fake_cluster | ||
| collector_endpoint: /api/v2/spans | ||
| collector_endpoint_version: HTTP_JSON | ||
| )EOF"; | ||
|
|
||
| envoy::config::trace::v3::ZipkinConfig zipkin_config; | ||
| TestUtility::loadFromYaml(yaml_string, zipkin_config); | ||
|
|
||
| setup(zipkin_config, true); | ||
| EXPECT_NE(nullptr, driver_); | ||
| } |
Member
There was a problem hiding this comment.
These tests should be removed because it have been tested in the config test.
Comment on lines
1140
to
1176
| TEST_F(ZipkinDriverTest, ReporterFlushWithCustomHeaders) { | ||
| setupValidDriverWithHostname("HTTP_JSON", ""); | ||
|
|
||
| Http::MockAsyncClientRequest request(&cm_.thread_local_cluster_.async_client_); | ||
| Http::AsyncClient::Callbacks* callback; | ||
| const absl::optional<std::chrono::milliseconds> timeout(std::chrono::seconds(5)); | ||
|
|
||
| // Set up expectations for the HTTP request | ||
| EXPECT_CALL(cm_.thread_local_cluster_.async_client_, | ||
| send_(_, _, Http::AsyncClient::RequestOptions().setTimeout(timeout))) | ||
| .WillOnce( | ||
| Invoke([&](Http::RequestMessagePtr& message, Http::AsyncClient::Callbacks& callbacks, | ||
| const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { | ||
| callback = &callbacks; | ||
|
|
||
| // Verify standard headers are present | ||
| EXPECT_EQ("/api/v2/spans", message->headers().getPathValue()); | ||
| EXPECT_EQ("fake_cluster", message->headers().getHostValue()); | ||
| EXPECT_EQ("application/json", message->headers().getContentTypeValue()); | ||
|
|
||
| return &request; | ||
| })); | ||
|
|
||
| EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.zipkin.min_flush_spans", 5)) | ||
| .WillOnce(Return(1)); | ||
| EXPECT_CALL(runtime_.snapshot_, getInteger("tracing.zipkin.request_timeout", 5000U)) | ||
| .WillOnce(Return(5000U)); | ||
|
|
||
| Tracing::SpanPtr span = driver_->startSpan(config_, request_headers_, stream_info_, | ||
| operation_name_, {Tracing::Reason::Sampling, true}); | ||
| span->finishSpan(); | ||
|
|
||
| Http::ResponseHeaderMapPtr response_headers{ | ||
| new Http::TestResponseHeaderMapImpl{{":status", "202"}}}; | ||
| callback->onSuccess(request, | ||
| std::make_unique<Http::ResponseMessageImpl>(std::move(response_headers))); | ||
| } |
Member
There was a problem hiding this comment.
Also should be removed because this should tested be previous test cases.
| std::make_unique<Http::ResponseMessageImpl>(std::move(response_headers))); | ||
| } | ||
|
|
||
| TEST_F(ZipkinDriverTest, ReporterFlushWithHttpServiceHeadersProtobuf) { |
Member
There was a problem hiding this comment.
This test is unnecessary because the collector_service has no business with the body's type (json or protobuf).
Contributor
Author
There was a problem hiding this comment.
Done! Removed !
| "Either collector_cluster or collector_service must be specified"); | ||
| } | ||
|
|
||
| TEST_F(ZipkinDriverTest, DriverWithHttpServiceMissingCluster) { |
Member
There was a problem hiding this comment.
This is unnecessary. PGV should ensure this.
Comment on lines
1416
to
1417
| // Note: Test for missing URI was removed as we now rely on proto validation | ||
| // instead of runtime validation for required fields. |
Member
There was a problem hiding this comment.
I think this note should also be removed.
| // Note: Test for missing URI was removed as we now rely on proto validation | ||
| // instead of runtime validation for required fields. | ||
|
|
||
| TEST_F(ZipkinDriverTest, DriverLegacyConfigMissingEndpoint) { |
Member
There was a problem hiding this comment.
This should be covered by tests before this PR.
Contributor
Author
There was a problem hiding this comment.
Yes I removed the test
| "collector_endpoint must be specified when using collector_cluster"); | ||
| } | ||
|
|
||
| TEST_F(ZipkinDriverTest, DriverLegacyConfigWithHostname) { |
Member
There was a problem hiding this comment.
Also, this should be covered by tests before this PR. Even it didn't, we needn't to add that at this PR.
Contributor
Author
There was a problem hiding this comment.
Yes removed the test
…onfig, allowing users to add custom headers to HTTP requests sent to the Zipkin collector. This enables authentication, authorization, and collector-specific routing capabilities. The feature includes: - New collector_request_headers field in ZipkinConfig proto - CollectorInfo struct updated to store header key-value pairs - ReporterImpl modified to include headers in HTTP requests - Comprehensive test coverage for all components - Documentation updates in tracing overview and FAQ This is different from custom tags which add metadata to spans; this feature adds HTTP headers to collector API requests. Signed-off-by: Prashanth Josyula <[email protected]>
…onfig, allowing users to add custom headers to HTTP requests sent to the Zipkin collector. This enables authentication, authorization, and collector-specific routing capabilities. The feature includes: - New collector_request_headers field in ZipkinConfig proto - CollectorInfo struct updated to store header key-value pairs - ReporterImpl modified to include headers in HTTP requests - Comprehensive test coverage for all components - Documentation updates in tracing overview and FAQ This is different from custom tags which add metadata to spans; this feature adds HTTP headers to collector API requests. Signed-off-by: Prashanth Josyula <[email protected]>
…onfig, allowing users to add custom headers to HTTP requests sent to the Zipkin collector. This enables authentication, authorization, and collector-specific routing capabilities. The feature includes: - New collector_request_headers field in ZipkinConfig proto - CollectorInfo struct updated to store header key-value pairs - ReporterImpl modified to include headers in HTTP requests - Comprehensive test coverage for all components - Documentation updates in tracing overview and FAQ This is different from custom tags which add metadata to spans; this feature adds HTTP headers to collector API requests. Signed-off-by: Prashanth Josyula <[email protected]>
prashanthjos
commented
Sep 8, 2025
| source/extensions/stat_sinks/graphite_statsd: 82.8 # Death tests don't report LCOV | ||
| source/extensions/stat_sinks/statsd: 85.2 # Death tests don't report LCOV | ||
| source/extensions/tracers/zipkin: 95.4 | ||
| source/extensions/tracers/zipkin: 95.3 |
Contributor
Author
There was a problem hiding this comment.
there is 0.1% decrease in code coverage after we have consolidated the tests.
Contributor
Author
|
@wbpcode I have addressed all the comments, can you please review/approve. |
wbpcode
approved these changes
Sep 9, 2025
|
|
||
| if (!parsed_hostname.empty()) { | ||
| // Use the hostname from the URI | ||
| hostname_ = parsed_hostname; |
Member
There was a problem hiding this comment.
I have different opinion but anyway, this is a nit.
barroca
pushed a commit
to barroca/envoy
that referenced
this pull request
Sep 9, 2025
…#40863) <!-- !!!ATTENTION!!! If you are fixing *any* crash or *any* potential security issue, *do not* open a pull request in this repo. Please report the issue via emailing [email protected] where the issue will be triaged appropriately. Thank you in advance for helping to keep Envoy secure. !!!ATTENTION!!! For an explanation of how to fill out the fields, please see the relevant section in [PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/main/PULL_REQUESTS.md) --> Commit Message: Add support for custom HTTP headers in Zipkin collector requests This change introduces the collector_request_headers field to ZipkinConfig, allowing users to add custom headers to HTTP requests sent to the Zipkin collector. This enables authentication, authorization, and collector-specific routing capabilities. The feature includes: - New collector_request_headers field in ZipkinConfig proto - CollectorInfo struct updated to store header key-value pairs - ReporterImpl modified to include headers in HTTP requests - Comprehensive test coverage for all components - Documentation updates in tracing overview and FAQ This is different from custom tags which add metadata to spans; this feature adds HTTP headers to collector API requests. Additional Description: Risk Level: Medium Testing: Unit tests, integration tests, configuration tests Docs Changes: Updated tracing architecture docs and FAQ Release Notes: Yes - added to current.yaml --------- Signed-off-by: Prashanth Josyula <[email protected]> Signed-off-by: Prashanth Josyula <[email protected]> Co-authored-by: Prashanth Josyula <[email protected]> Signed-off-by: Leonardo da Mata <[email protected]>
mbadov
pushed a commit
to mbadov/envoy
that referenced
this pull request
Sep 22, 2025
…#40863) <!-- !!!ATTENTION!!! If you are fixing *any* crash or *any* potential security issue, *do not* open a pull request in this repo. Please report the issue via emailing [email protected] where the issue will be triaged appropriately. Thank you in advance for helping to keep Envoy secure. !!!ATTENTION!!! For an explanation of how to fill out the fields, please see the relevant section in [PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/main/PULL_REQUESTS.md) --> Commit Message: Add support for custom HTTP headers in Zipkin collector requests This change introduces the collector_request_headers field to ZipkinConfig, allowing users to add custom headers to HTTP requests sent to the Zipkin collector. This enables authentication, authorization, and collector-specific routing capabilities. The feature includes: - New collector_request_headers field in ZipkinConfig proto - CollectorInfo struct updated to store header key-value pairs - ReporterImpl modified to include headers in HTTP requests - Comprehensive test coverage for all components - Documentation updates in tracing overview and FAQ This is different from custom tags which add metadata to spans; this feature adds HTTP headers to collector API requests. Additional Description: Risk Level: Medium Testing: Unit tests, integration tests, configuration tests Docs Changes: Updated tracing architecture docs and FAQ Release Notes: Yes - added to current.yaml --------- Signed-off-by: Prashanth Josyula <[email protected]> Signed-off-by: Prashanth Josyula <[email protected]> Co-authored-by: Prashanth Josyula <[email protected]> Signed-off-by: Misha Badov <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commit Message: Add support for custom HTTP headers in Zipkin collector requests
This change introduces the collector_request_headers field to ZipkinConfig,
allowing users to add custom headers to HTTP requests sent to the Zipkin
collector. This enables authentication, authorization, and collector-specific
routing capabilities.
The feature includes:
This is different from custom tags which add metadata to spans;
this feature adds HTTP headers to collector API requests.
Additional Description:
Risk Level: Medium
Testing: Unit tests, integration tests, configuration tests
Docs Changes: Updated tracing architecture docs and FAQ
Release Notes: Yes - added to current.yaml