New ECS health check extension #5144

skyduo · 2021-09-07T20:27:54Z

Description:
This is a new health check extension feature for AWS/ECS, as we are going to monitor the OT collector health through the return status from endpoint of the new extension, detailed design in the design doc below.

Link to tracking Issue:
open-telemetry/opentelemetry-collector#2573

Testing:
make otelcontribcol and run executable file

Documentation:
https://docs.google.com/document/d/1SpUMsWA2DeaoVazeQ8uEc1Wvu5LphmQU_TjzLmuJ4QM/edit#heading=h.rs1luwizct2w

original pr which has some discussion with @bogdandrutu and previous pr #4451

# Conflicts: # go.mod

# Conflicts: # go.mod # internal/components/components.go

# Conflicts: # extension/awsecshealthcheckextension/go.mod # extension/awsecshealthcheckextension/go.sum # internal/components/components.go

gillg · 2021-09-07T21:09:33Z

Hello, open question about this interesting new extension !
What is ECS specific here ? It could be a "smart health check" or something like that to avoid confusion no ? Doesn't it can be used outside ECS just behind a load balancer, or in kubernetes, or in swarm ?

hossain-rayhan

LGTM.

skyduo · 2021-09-08T00:17:55Z

Hello, open question about this interesting new extension !
What is ECS specific here ? It could be a "smart health check" or something like that to avoid confusion no ? Doesn't it can be used outside ECS just behind a load balancer, or in kubernetes, or in swarm ?

so basically OT already has a health check extension for general usage, and we create this new one because we want to set some specific healthy condition for ECS, that's why it's a ECS health check extension.

anuraaga · 2021-09-08T02:18:12Z

@skyduo This seems to be a copy of #4451? Please avoid fragmenting discussion by opening duplicate PRs, we should probably close this one.

skyduo · 2021-09-08T17:34:29Z

@skyduo This seems to be a copy of #4451? Please avoid fragmenting discussion by opening duplicate PRs, we should probably close this one.

hey Anuraag! i will close the previous one due to that one has some rebase issue

extension/awsecshealthcheckextension/Makefile

extension/awsecshealthcheckextension/config.go

extension/awsecshealthcheckextension/exporter_test.go

Aneurysm9 · 2021-09-08T22:56:17Z

extension/awsecshealthcheckextension/extension.go

+}
+
+func (hc *ecsHealthCheckExtension) Start(_ context.Context, host component.Host) error {
+	hc.logger.Info("Starting ECS health check extension", zap.Any("config", hc.config))


Does this really need to emit the entire config at Info? Maybe have a separate entry with it at Debug if it is useful?

i think it's fine, just tell the user this extension starts

I'll restate:

This does not need to emit the entire config at Info? Have a separate entry with it at Debug if it is useful?

extension/awsecshealthcheckextension/extension.go

Aneurysm9 · 2021-09-08T23:04:57Z

extension/awsecshealthcheckextension/extension.go

+	hc.exporter.mu.Lock()
+	defer hc.exporter.mu.Unlock()
+
+	return hc.config.ExporterErrorLimit >= len(hc.exporter.exporterErrorQueue)


Don't access internals of the exporter like this. Add an accessor that can handle the locking itself.

this exporter is embedded in the extension and not used anywhere else, i think it's OK to lock and unlock like this.

That may be the current situation, but it is not good practice and it may not always be the case. If it is changed in the future then the person who changes it may not be aware of this use.

extension/awsecshealthcheckextension/factory.go

extension/awsecshealthcheckextension/extension.go

anuraaga · 2021-09-08T23:16:12Z

In that case, let me copy in my comment. I think there is a recurring theme that people wonder why we are adding this extension since it doesn't have ECS-specific logic at all. We should all work together to improve the default health check instead as a community.

There doesn't seem to be anything ECS-specific in this code. Shouldn't the ability to become not-ready when there are an excessive number of exporter failures be added to the normal healthcheckextension instead of a new extension? This is broadly applicable.
Related is there is interest in rewriting that extension to use a system such as https://github.com/alexliesenfeld/health @jpkrohling @bogdandrutu

…tor-contrib into main

# Conflicts: # cmd/configschema/go.mod # go.mod

gillg · 2021-09-12T20:14:00Z

We should all work together to improve the default health check instead as a community.

Moreover, healthcheckextension is now on contrib instead of core, so it's probably more acceptable to make some eventual breaking changes to make it more complete and adapted to any situation.
Also I always don't understand what is specific with ECS. ECS use the native docker mechanism for container healthcheck, based on a shell command executed at container (CMD or CMD-SHELL) level. It can be a curl "ping" or anything else. If the helthckeck is used for a LoadBalancer used by ECS, it's just a ping made by the LB to an http target with a certain error thresold. Does someone can precise the exact need for ECS, missed by the base healthcheck ?

…tor-contrib into main

Aneurysm9

I have to echo the sentiments of @anuraaga and others. I don't see why this is positioned as an ECS health check and not an extension to the general health check. This provides a seemingly general capability and should not take the risk of deterring potential users by mislabeling itself.

alolita · 2021-09-17T17:25:41Z

@skyduo i agree with @Aneurysm9 @anuraaga and others that this is a good addition to the existing healthcheck extension. I don't think it needs to be AWS ECS specific. We don't want to be in the business of maintaining custom components wherever possible.

PettitWesley · 2021-09-17T17:50:26Z

@alolita @Aneurysm9 @anuraaga @skyduo I agree that this should be an addition/extra feature inside the existing health check extension. IIRC, this is what we first proposed when we first ever discussed our need/use case in a SIG meeting. However, the exact use case of making the health check fail if the exporter has errors was said to be very ECS specific by the folks in that meeting. So we were asked to pursue this route. If I remember correct, @tigrannajaryan and @bogdandrutu recommended that approach. I am happy if the community prefers we put all health check functionality in one extension. But want to double check that all stakeholders have weighed in.

skyduo · 2021-09-17T18:14:36Z

yeah i have no objection on making it a general one, just like Wesley said if everyone agree with it

alolita · 2021-09-19T03:48:42Z

@PettitWesley @skyduo - thx for clarifying. Will bring this up for discussion w maintainers.

Aneurysm9 · 2021-09-22T19:17:31Z

extension/awsecshealthcheckextension/extension.go

+// initExporter function could register the customized exporter
+func (hc *ecsHealthCheckExtension) initExporter() error {
+	hc.exporter = newECSHealthCheckExporter()
+	view.RegisterExporter(hc.exporter)


Can this be unregistered when the extension is shut down?

skyduo · 2021-09-24T21:57:44Z

based on the discussion in the SIG meeting, we will move new health check strategy to the existing health check extension.

github-actions · 2021-10-02T05:15:17Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

jpkrohling · 2021-10-06T11:48:30Z

Should this be closed in favor of the other PR?

github-actions · 2021-10-14T05:15:30Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

jpkrohling · 2021-10-14T07:58:00Z

I'm closing this in favor of the other PR.

Because, most likely for the moment only otlp exporter/receiver is using this package, we can remove this after just one version, no third-party deps. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

skyduo added 17 commits August 4, 2021 22:45

Add new AWS ECS Health Check Extension

45e83e8

Modify the order of import and new extension factory

63eae1a

Modify interval read methods and config validation check.

d4aa797

Merge branch 'main' into healthcheck

9c18d80

# Conflicts: # go.mod

Modify error message and go.mod

74ab2cf

Create readme of the new health check extension.

e66f7f1

Modify unit test based on the new view name.

fc92a50

Merge branch 'main' into healthcheck

caf27b9

# Conflicts: # go.mod # internal/components/components.go

Resolve conflict files.

3d48604

Merge remote-tracking branch 'origin/main' into healthcheck

b48b664

# Conflicts: # go.mod # internal/components/components.go

Modify extension validation and initiation logic and add go.mod

4898ab3

Merge remote-tracking branch 'fork/main' into healthcheck

fc749e2

# Conflicts: # go.mod # internal/components/components.go

Modify extension logic and go.mod

973e63a

Init go.mod for health check extension

fa5e409

Merge branch 'main' into healthcheck

cbfc075

# Conflicts: # extension/awsecshealthcheckextension/go.mod # extension/awsecshealthcheckextension/go.sum # internal/components/components.go

Update new health check extension version

ddf3c77

Add new makefile and extension into dependabot

3fcab95

skyduo requested review from a team and owais September 7, 2021 20:27

github-actions bot assigned djaglowski Sep 7, 2021

hossain-rayhan approved these changes Sep 7, 2021

View reviewed changes

Modify unit tests and go mod

f58c013

Aneurysm9 reviewed Sep 8, 2021

View reviewed changes

Modify extension and config description.

309889a

skyduo added 4 commits September 10, 2021 14:21

Merge branch 'main' of https://github.com/skyduo/opentelemetry-collec…

bbdc983

…tor-contrib into main

Merge branch 'main' into newhealthcheck

c9a0215

# Conflicts: # cmd/configschema/go.mod # go.mod

Update go mod/sum versions

37bb76a

Update go mod and sum versions

7a3d02c

skyduo added 2 commits September 15, 2021 17:14

upgrade go version to 1.17

faf65eb

Merge branch 'main' of https://github.com/skyduo/opentelemetry-collec…

3f26570

…tor-contrib into main

skyduo force-pushed the newhealthcheck branch 2 times, most recently from 8321cd0 to ff20722 Compare September 16, 2021 16:55

Update go mod

bdad8cf

skyduo force-pushed the newhealthcheck branch from ff20722 to bdad8cf Compare September 16, 2021 17:16

Aneurysm9 suggested changes Sep 17, 2021

View reviewed changes

Aneurysm9 reviewed Sep 22, 2021

View reviewed changes

github-actions bot added the Stale label Oct 2, 2021

skyduo mentioned this pull request Oct 5, 2021

Modify health check extension to add new metrics health check functions #5643

Merged

bogdandrutu removed the Stale label Oct 6, 2021

github-actions bot added the Stale label Oct 14, 2021

jpkrohling closed this Oct 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New ECS health check extension #5144

New ECS health check extension #5144

skyduo commented Sep 7, 2021

gillg commented Sep 7, 2021

hossain-rayhan left a comment

skyduo commented Sep 8, 2021 •

edited

Loading

anuraaga commented Sep 8, 2021

skyduo commented Sep 8, 2021

Aneurysm9 Sep 8, 2021

skyduo Sep 9, 2021

Aneurysm9 Sep 17, 2021

Aneurysm9 Sep 8, 2021

skyduo Sep 9, 2021

Aneurysm9 Sep 17, 2021

anuraaga commented Sep 8, 2021

gillg commented Sep 12, 2021 •

edited

Loading

Aneurysm9 left a comment

alolita commented Sep 17, 2021

PettitWesley commented Sep 17, 2021

skyduo commented Sep 17, 2021

alolita commented Sep 19, 2021

Aneurysm9 Sep 22, 2021

skyduo commented Sep 24, 2021

github-actions bot commented Oct 2, 2021

jpkrohling commented Oct 6, 2021

github-actions bot commented Oct 14, 2021

jpkrohling commented Oct 14, 2021

New ECS health check extension #5144

New ECS health check extension #5144

Conversation

skyduo commented Sep 7, 2021

gillg commented Sep 7, 2021

hossain-rayhan left a comment

Choose a reason for hiding this comment

skyduo commented Sep 8, 2021 • edited Loading

anuraaga commented Sep 8, 2021

skyduo commented Sep 8, 2021

Aneurysm9 Sep 8, 2021

Choose a reason for hiding this comment

skyduo Sep 9, 2021

Choose a reason for hiding this comment

Aneurysm9 Sep 17, 2021

Choose a reason for hiding this comment

Aneurysm9 Sep 8, 2021

Choose a reason for hiding this comment

skyduo Sep 9, 2021

Choose a reason for hiding this comment

Aneurysm9 Sep 17, 2021

Choose a reason for hiding this comment

anuraaga commented Sep 8, 2021

gillg commented Sep 12, 2021 • edited Loading

Aneurysm9 left a comment

Choose a reason for hiding this comment

alolita commented Sep 17, 2021

PettitWesley commented Sep 17, 2021

skyduo commented Sep 17, 2021

alolita commented Sep 19, 2021

Aneurysm9 Sep 22, 2021

Choose a reason for hiding this comment

skyduo commented Sep 24, 2021

github-actions bot commented Oct 2, 2021

jpkrohling commented Oct 6, 2021

github-actions bot commented Oct 14, 2021

jpkrohling commented Oct 14, 2021

skyduo commented Sep 8, 2021 •

edited

Loading

gillg commented Sep 12, 2021 •

edited

Loading