[receiver/zipkin] add protocols option #40277
Conversation
EOjeah
requested review from
MovieStoreGuy,
andrzej-stencel,
crobert-1 and
a team
as code owners
- Prevent unkeyed literal initialization in HTTPConfig - Populate protocols with HTTP config during unmarshaling - Improve logging for HTTP server startup
…rib into u/eojeah/add-protocols-option-zipkin-receiver
There was a problem hiding this comment.
Thanks @EOjeah for this contribution. There are a couple things that need changing.
-
We must never use
fmt.Printlnfor logging. We must only use the built-in logger for this. -
I believe the component should only accept one of the HTTP configurations: either top-level or protocols. I've just described how I see it in the issue: Refactor Zipkin Receiver to include Protocols in Config #35730 (comment). Let's continue the conversation in the issue and move on with the implementation when we agree on how config validation should work.
| @@ -29,9 +29,44 @@ receivers: | |||
|
|
|||
| The following settings are configurable: | |||
|
|
|||
| - `endpoint` (default = localhost:9411): host:port on which the receiver is going to receive data.See our [security best practices doc](https://opentelemetry.io/docs/security/config-best-practices/#protect-against-denial-of-service-attacks) to understand how to set the endpoint in different environments. You can review the [full list of `ServerConfig`](https://github.com/open-telemetry/opentelemetry-collector/tree/main/config/confighttp). | |||
| ### Legacy Configuration (Deprecated) | |||
There was a problem hiding this comment.
We should describe the recommended configuration first and legacy after that.
| - `endpoint` (default = localhost:9411): host:port on which the receiver is going to receive data.See our [security best practices doc](https://opentelemetry.io/docs/security/config-best-practices/#protect-against-denial-of-service-attacks) to understand how to set the endpoint in different environments. You can review the [full list of `ServerConfig`](https://github.com/open-telemetry/opentelemetry-collector/tree/main/config/confighttp). | ||
| ### Legacy Configuration (Deprecated) | ||
|
|
||
| - `endpoint` (default = localhost:9411): host:port on which the receiver is going to receive data. See our [security best practices doc](https://opentelemetry.io/docs/security/config-best-practices/#protect-against-denial-of-service-attacks) to understand how to set the endpoint in different environments. For full list of `ServerConfig` refer [here](https://github.com/open-telemetry/opentelemetry-collector/tree/main/config/confighttp). | ||
| - `parse_string_tags` (default = false): if enabled, the receiver will attempt to parse string tags/binary annotations into int/bool/float. |
There was a problem hiding this comment.
parse_string_tags is not a legacy option, it's description should be included in the main (non-legacy) configuration description.
| - `parse_string_tags` (default = false): if enabled, the receiver will attempt to parse string tags/binary annotations into int/bool/float. | ||
|
|
||
| ### Protocol-Based Configuration | ||
|
|
||
| Starting from v0.128.0, the zipkin receiver supports a protocol-based configuration format: |
There was a problem hiding this comment.
No need to mention in the docs the version an option appeared in. That's what the Changelog is for.
| parse_string_tags: false | ||
| ``` | ||
|
|
||
| Currently, only the HTTP protocol is supported, but this structure allows for future expansion to other protocols (such as UDP). The protocol-based design makes the receiver more extensible and follows the pattern used by other receivers in the OpenTelemetry Collector. |
There was a problem hiding this comment.
I say let's not bother the users with these details here. They just want to configure the component.
| Currently, only the HTTP protocol is supported, but this structure allows for future expansion to other protocols (such as UDP). The protocol-based design makes the receiver more extensible and follows the pattern used by other receivers in the OpenTelemetry Collector. | |
| Currently, only the HTTP protocol is supported, but this structure allows for future expansion to other protocols (such as UDP). |
| #### Future Protocol Support | ||
|
|
||
| The architecture has been designed to easily add support for additional protocols in the future. For example, a UDP protocol could be added with configuration like: | ||
|
|
||
| ```yaml | ||
| receivers: | ||
| zipkin: | ||
| protocols: | ||
| http: | ||
| endpoint: localhost:9411 | ||
| # UDP support is not yet implemented, this is just an example | ||
| # udp: | ||
| # endpoint: localhost:9412 | ||
| parse_string_tags: false | ||
| ``` |
There was a problem hiding this comment.
Let's not make "forward looking statements" describing what might be possible in the future. Users might be confused on whether UDP is currently supported or not.
| #### Future Protocol Support | |
| The architecture has been designed to easily add support for additional protocols in the future. For example, a UDP protocol could be added with configuration like: | |
| ```yaml | |
| receivers: | |
| zipkin: | |
| protocols: | |
| http: | |
| endpoint: localhost:9411 | |
| # UDP support is not yet implemented, this is just an example | |
| # udp: | |
| # endpoint: localhost:9412 | |
| parse_string_tags: false | |
| ``` |
| // If protocols section exists, clear the legacy endpoint to avoid confusion | ||
| if cfg.Endpoint != "" && conf.IsSet(protoHTTP) { | ||
| // Both legacy and new configuration used, log a warning | ||
| fmt.Println("Warning: Both legacy endpoint and protocols.http configuration found. Using protocols.http configuration.") |
There was a problem hiding this comment.
🛑 We must never use fmt.Println for logging.
andrzej-stencel
changed the title
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Description
This PR refactors the zipkin receiver to include protocols in the configuration, making it more consistent with other receivers in the collector and extensible for future protocols. The changes include:
Added a protocol-based configuration structure that allows users to configure the zipkin receiver using the protocols section:
Maintained backward compatibility with the legacy configuration format:
Implemented a modular architecture that separates protocol handling, making it easier to add new protocols in the future (such as UDP).
Added clear warnings when both legacy and protocol-based configurations are used, prioritizing the protocol-based configuration.
Link to tracking issue
Fixes #35730
Testing
Updated all existing tests to work with the new configuration structure
Added specific test cases for both legacy and protocol-based configurations
Added test cases for configurations with both legacy and protocol settings
Ensured all tests pass with the new implementation
Verified backward compatibility with existing configurations
Documentation
Updated the README.md with examples of the new protocol-based configuration
Added documentation about future protocol extensibility
Included examples of how both legacy and new configuration formats work
Added comments in the code explaining the design decisions and how to extend the receiver with new protocols