Create a new AWS ECS Health Check Extension #4451

skyduo · 2021-08-05T05:53:12Z

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 code is almost same as this pr:
open-telemetry/opentelemetry-collector#3614

linux-foundation-easycla · 2021-08-05T05:53:15Z

The committers are authorized under a signed CLA.

✅ skyduo (45e83e8, 63eae1a)

skyduo · 2021-08-11T22:12:55Z

Hi @bogdandrutu ! could you help review this pr? thanks!

hossain-rayhan · 2021-08-13T20:42:02Z

@Aneurysm9 can you please help to review this one from AWS side? Thanks.
CC: @alolita

extension/awsecshealthcheckextension/config.go

extension/awsecshealthcheckextension/extension.go

extension/awsecshealthcheckextension/config.go

extension/awsecshealthcheckextension/factory.go

hossain-rayhan

Please add a README explaining the config and rotation mechanism.

# Conflicts: # go.mod

bogdandrutu · 2021-08-19T20:59:50Z

Please rebase :)

extension/awsecshealthcheckextension/exporter.go

extension/awsecshealthcheckextension/extension.go

extension/awsecshealthcheckextension/factory.go

extension/awsecshealthcheckextension/extension.go

extension/awsecshealthcheckextension/exporter.go

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

skyduo · 2021-08-20T22:05:11Z

Hey @bogdandrutu ! please have a review again, and point out if any changes needed.

github-actions · 2021-09-01T05:15:23Z

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

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

PettitWesley · 2021-09-02T23:32:27Z

This should not be marked as stale, we still want it released.

anuraaga · 2021-09-03T03:21:16Z

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

jpkrohling · 2021-09-08T09:43:43Z

Related is there is interest in rewriting that extension to use a system such as https://github.com/alexliesenfeld/health

Yes, I would prefer to have this in place of the current health check extension.

github-actions · 2021-09-16T05:15:24Z

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

github-actions · 2021-10-01T05:15:23Z

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

github-actions · 2021-10-15T05:15:24Z

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

github-actions · 2021-10-24T05:15:27Z

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

MovieStoreGuy · 2021-11-08T09:48:11Z

extension/awsecshealthcheckextension/config_test.go

+
+func TestLoadConfig(t *testing.T) {
+	factories, err := componenttest.NopFactories()
+	assert.NoError(t, err)


Suggested change

assert.NoError(t, err)

require.NoError(t, err)

If this fails, it would create a flaky test with a nil pointer exception

MovieStoreGuy · 2021-11-08T09:50:39Z

extension/awsecshealthcheckextension/exporter.go

+
+// ECSHealthCheckExporter is a struct implement the exporter interface in open census that could export metrics
+type ecsHealthCheckExporter struct {
+	mu                 sync.Mutex


Suggested change

mu sync.Mutex

rw sync.RWMutex

Using a RW Mutex will allow better performance.

MovieStoreGuy · 2021-11-08T09:54:38Z

extension/awsecshealthcheckextension/exporter.go

+
+	viewNum := len(e.exporterErrorQueue)
+	currentTime := time.Now()
+	for i := 0; i < viewNum; i++ {


This looks like it is managing a sliding window so index values could be stored inside the struct, improving performance.

MovieStoreGuy · 2021-11-08T09:55:57Z

extension/awsecshealthcheckextension/exporter_test.go

+	"testing"
+	"time"
+
+	"gotest.tools/v3/assert"


Could i ask you https://pkg.go.dev/github.com/stretchr/testify/assert instead? It heavily used within the project and keeps testing patterns the same.

MovieStoreGuy · 2021-11-08T09:59:47Z

extension/awsecshealthcheckextension/exporter_test.go

+)
+
+func TestECSHealthCheckExporter_ExportView(t *testing.T) {
+	exporter := &ecsHealthCheckExporter{}


Could I ask you to us t.Parallel() in tests?

Would allow to speed up testing throughout the project 🙏🏽

MovieStoreGuy · 2021-11-08T10:07:19Z

extension/awsecshealthcheckextension/extension.go

+		default:
+		}
+
+		if err := hc.server.Serve(ln); err != http.ErrServerClosed && err != nil {


Would it be better to use errors.Is ?

MovieStoreGuy · 2021-11-08T10:09:02Z

extension/awsecshealthcheckextension/extension_test.go

+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+
+	"go.opencensus.io/stats/view"


Is this correctly "import-ed" ?

MovieStoreGuy · 2021-11-08T10:10:00Z

extension/awsecshealthcheckextension/extension_test.go

+		ExporterErrorLimit: 1,
+	}
+
+	hcExt := newServer(config, zap.NewNop())


Suggested change

hcExt := newServer(config, zap.NewNop())

hcExt := newServer(config, zaptest.NewTestLogger(t))

MovieStoreGuy · 2021-11-08T10:11:18Z

extension/awsecshealthcheckextension/extension_test.go

+}
+
+func TestHealthCheckExtensionPortAlreadyInUse(t *testing.T) {
+	endpoint := "localhost:13134"


This has potential to cause bind issues since it matches above, would be better to use httptest.NewServer ?

MovieStoreGuy · 2021-11-08T10:12:17Z

extension/awsecshealthcheckextension/extension_test.go

+		Interval:           "5m",
+		ExporterErrorLimit: 5,
+	}
+	hcExt := newServer(config, zap.NewNop())


Suggested change

hcExt := newServer(config, zap.NewNop())

hcExt := newServer(config, zaptest.NewTestLogger(t))

MovieStoreGuy · 2021-11-08T10:33:14Z

Can you resolve the conflicts please?

github-actions · 2021-12-04T05:15:40Z

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

@MovieStoreGuy

Comments from @MovieStoreGuy require reassessment.

github-actions · 2021-12-14T05:15:38Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

* `configgrpc`: Update `ToDialOptions` with settings Adding the ability to pass TelemetrySettings to ToDialOptions to configure underlying instrumentation library. Fixes open-telemetry#4424 * update changelog * add telemetrysetting to exporter

Add new AWS ECS Health Check Extension

45e83e8

skyduo requested review from a team and owais August 5, 2021 05:53

github-actions bot assigned bogdandrutu Aug 5, 2021

Modify the order of import and new extension factory

63eae1a

skyduo force-pushed the healthcheck branch from 77c6778 to 63eae1a Compare August 9, 2021 19:23

skyduo mentioned this pull request Aug 13, 2021

Add new view for AWS ECS health check extension. open-telemetry/opentelemetry-collector#3776

Merged

Aneurysm9 reviewed Aug 16, 2021

View reviewed changes

extension/awsecshealthcheckextension/config.go Outdated Show resolved Hide resolved

extension/awsecshealthcheckextension/extension.go Outdated Show resolved Hide resolved

hossain-rayhan reviewed Aug 16, 2021

View reviewed changes

extension/awsecshealthcheckextension/config.go Outdated Show resolved Hide resolved

Modify interval read methods and config validation check.

d4aa797

skyduo requested a review from Aneurysm9 August 18, 2021 22:58

Aneurysm9 previously approved these changes Aug 18, 2021

View reviewed changes

hossain-rayhan reviewed Aug 18, 2021

View reviewed changes

extension/awsecshealthcheckextension/config.go Outdated Show resolved Hide resolved

hossain-rayhan reviewed Aug 18, 2021

View reviewed changes

extension/awsecshealthcheckextension/factory.go Show resolved Hide resolved

hossain-rayhan suggested changes Aug 18, 2021

View reviewed changes

skyduo added 3 commits August 18, 2021 17:26

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

bogdandrutu requested changes Aug 19, 2021

View reviewed changes

skyduo added 3 commits August 20, 2021 14:22

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

hossain-rayhan approved these changes Aug 20, 2021

View reviewed changes

skyduo requested a review from bogdandrutu August 24, 2021 22:37

skyduo added 3 commits September 2, 2021 11:24

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

c192310

github-actions bot removed the Stale label Sep 3, 2021

skyduo mentioned this pull request Sep 7, 2021

New ECS health check extension #5144

Closed

anuraaga requested a review from jpkrohling September 8, 2021 02:18

github-actions bot added the Stale label Sep 16, 2021

bogdandrutu removed the Stale label Sep 22, 2021

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

bogdandrutu removed the Stale label Oct 6, 2021

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

bogdandrutu removed the Stale label Oct 15, 2021

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

bogdandrutu removed the Stale label Oct 29, 2021

MovieStoreGuy reviewed Nov 8, 2021

View reviewed changes

github-actions bot added the Stale label Dec 4, 2021

Aneurysm9 self-requested a review December 6, 2021 22:33

github-actions bot closed this Dec 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a new AWS ECS Health Check Extension #4451

Create a new AWS ECS Health Check Extension #4451

skyduo commented Aug 5, 2021

linux-foundation-easycla bot commented Aug 5, 2021 •

edited

Loading

skyduo commented Aug 11, 2021

hossain-rayhan commented Aug 13, 2021

hossain-rayhan left a comment

bogdandrutu commented Aug 19, 2021

skyduo commented Aug 20, 2021

github-actions bot commented Sep 1, 2021

PettitWesley commented Sep 2, 2021

anuraaga commented Sep 3, 2021

jpkrohling commented Sep 8, 2021

github-actions bot commented Sep 16, 2021

github-actions bot commented Oct 1, 2021

github-actions bot commented Oct 15, 2021

github-actions bot commented Oct 24, 2021

MovieStoreGuy Nov 8, 2021

MovieStoreGuy Nov 8, 2021

MovieStoreGuy Nov 8, 2021

MovieStoreGuy Nov 8, 2021

MovieStoreGuy Nov 8, 2021

MovieStoreGuy Nov 8, 2021

MovieStoreGuy Nov 8, 2021

MovieStoreGuy Nov 8, 2021

MovieStoreGuy Nov 8, 2021

MovieStoreGuy Nov 8, 2021

MovieStoreGuy commented Nov 8, 2021

github-actions bot commented Dec 4, 2021

github-actions bot commented Dec 14, 2021

	hcExt := newServer(config, zap.NewNop())
	hcExt := newServer(config, zaptest.NewTestLogger(t))

Create a new AWS ECS Health Check Extension #4451

Create a new AWS ECS Health Check Extension #4451

Conversation

skyduo commented Aug 5, 2021

linux-foundation-easycla bot commented Aug 5, 2021 • edited Loading

skyduo commented Aug 11, 2021

hossain-rayhan commented Aug 13, 2021

hossain-rayhan left a comment

Choose a reason for hiding this comment

bogdandrutu commented Aug 19, 2021

skyduo commented Aug 20, 2021

github-actions bot commented Sep 1, 2021

PettitWesley commented Sep 2, 2021

anuraaga commented Sep 3, 2021

jpkrohling commented Sep 8, 2021

github-actions bot commented Sep 16, 2021

github-actions bot commented Oct 1, 2021

github-actions bot commented Oct 15, 2021

github-actions bot commented Oct 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MovieStoreGuy commented Nov 8, 2021

github-actions bot commented Dec 4, 2021

github-actions bot commented Dec 14, 2021

linux-foundation-easycla bot commented Aug 5, 2021 •

edited

Loading