Use Endpoint for health check extention #2782
Conversation
| // Port is the port used to publish the health check status. | ||
| // The default value is 13133. | ||
| Port uint16 `mapstructure:"port"` | ||
| // Endpoint is the address and port used to publish the health check status. |
There was a problem hiding this comment.
Can we do this change in two stages? Add the new one and deprecate the old option in one step, remove the deprecated in the next step. This is the type of change that doesn't need to be breaking for current users.
There was a problem hiding this comment.
Good point, I'll bring it back next commit
Codecov Report
@@ Coverage Diff @@
## main #2782 +/- ##
==========================================
- Coverage 91.64% 91.62% -0.03%
==========================================
Files 294 294
Lines 15637 15647 +10
==========================================
+ Hits 14331 14336 +5
- Misses 896 899 +3
- Partials 410 412 +2
Continue to review full report at Codecov.
|
|
Not sure, I just restarted the build. |
|
|
||
| // Endpoint is the address and port used to publish the health check status. | ||
| // The default value is "localhost:13133". | ||
| Endpoint string `mapstructure:"endpoint"` |
There was a problem hiding this comment.
Use confignet:
TcpAddr confignet.TcpAddr `mapstructure:",squash"`
There was a problem hiding this comment.
Fixed, I'd also suggest using confignet.TCPAddr instead of string in other extensions for the sake of consistency e.x:
https://github.com/open-telemetry/opentelemetry-collector/blob/main/extension/pprofextension/config.go#L29
I can fix it next PR.
|
Please rebase |
dstdfx
force-pushed
the
use-endpoint-hc-ext
branch
from
6ef287f to
8e77382
Compare
| @@ -41,8 +39,7 @@ func (hc *healthCheckExtension) Start(_ context.Context, host component.Host) er | |||
| hc.logger.Info("Starting health_check extension", zap.Any("config", hc.config)) | |||
|
|
|||
| // Initialize listener | |||
| portStr := ":" + strconv.Itoa(int(hc.config.Port)) | |||
| ln, err := net.Listen("tcp", portStr) | |||
| ln, err := hc.config.TCPAddr.Listen() | |||
There was a problem hiding this comment.
I think we need to do some backwards compatible things here correct?
if port != 0 and endpoint == default then endpoint = ":" + port
| ln net.Listener | ||
| err error | ||
| ) | ||
| if hc.config.Port != 0 && hc.config.TCPAddr.Endpoint == defaultEndpoint { |
There was a problem hiding this comment.
Also log a deprecation warning for users to change to endpoint :)
|
@dstdfx last request :))) can you please add a changelog entry for this? |
Signed-off-by: Daniil Rutskiy <[email protected]>
Signed-off-by: Daniil Rutskiy <[email protected]>
Signed-off-by: Daniil Rutskiy <[email protected]>
Signed-off-by: Daniil Rutskiy <[email protected]>
Signed-off-by: Daniil Rutskiy <[email protected]>
Signed-off-by: Daniil Rutskiy <[email protected]>
Signed-off-by: Daniil Rutskiy <[email protected]>
Signed-off-by: Daniil Rutskiy <[email protected]>
dstdfx
force-pushed
the
use-endpoint-hc-ext
branch
from
d341c69 to
6019f8a
Compare
|
Some fmt issues with imports: |
Signed-off-by: Daniil Rutskiy <[email protected]>
This commits brings back default configuration of the health_check extension to make sure it works in k8s-like environments without extra configuration as it was before the latest change open-telemetry#2782. Resolves: open-telemetry#2827
This commits brings back default configuration of the health_check extension to make sure it works in k8s-like environments without extra configuration as it was before the latest change open-telemetry#2782. Resolves: open-telemetry#2827
Description:
Change health check extension's config to use
Endpointinstead ofPort, fix tests according to the changes.Link to tracking Issue: #2764