Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create new AWS ECS health check extension. #3614

Closed
wants to merge 3 commits into from

Conversation

skyduo
Copy link
Contributor

@skyduo skyduo commented Jul 13, 2021

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, we will move this extension to opentelemetry-collector-contrib repo after we have can get the obsmetrics from internal folder in opentelemetry-collector.

Link to tracking Issue:
#2573

Testing:
make otelcol
./otelcol_darwin_amd64 --config /Users/tianduo/Documents/ot_healthcheck.yaml
run some script to connect to the endpoint, it will return 200/500 when the error number less/larger than the limit

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

@skyduo skyduo requested review from a team and tigrannajaryan July 13, 2021 23:14
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 13, 2021

CLA Not Signed

Comment on lines +44 to +52
if err := view.Register(&view.View{
Name: "AOC/ECSHealthCheckExporterFailedToSendSpans",
Description: "number of errors in exporters over span",
Measure: obsmetrics.ExporterFailedToSendSpans,
Aggregation: view.Count(),
}); err != nil {
log.Fatalf("Failed to register view: %v", err)
return err
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the only code that needs to access the internal package?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bogdandrutu yeah, i think this is the only thing we need to get from internal

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not simply just adding to https://github.com/open-telemetry/opentelemetry-collector/blob/main/internal/obsreportconfig/obsmetrics/obs_exporter.go as a generic metric, then you don't need to do anything else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @bogdandrutu ! i checked and found that i can add our new view here https://github.com/open-telemetry/opentelemetry-collector/blob/main/internal/obsreportconfig/obsreportconfig.go#L104-L105 due to we have to use view.Count(), so the code will be:

errorNumberView := &view.View{
		Name:        "ExporterFailedNumber",
		Description: "number of errors in exporters over span",
		Measure:     obsmetrics.ExporterFailedToSendSpans,
		Aggregation: view.Count(),
	}
views = append(views, errorNumberView)

is it correct? if so, i will push a new pr for this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this not enough? #3769?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't access that view anywhere else

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @bogdandrutu! so we have the new view in obsmetrics in the main repo and where is this metric sent to? we need to collect this new view in our new health check extension. We cannot even get the name of our new view now.

Our original code was:

if err := view.Register(&view.View{
		Name:        "AOC/ECSHealthCheckExporterFailedToSendSpans",
		Description: "number of errors in exporters over span",
		Measure:     obsmetrics.ExporterFailedToSendSpans,
		Aggregation: view.Count(),
	}); err != nil {
		log.Fatalf("Failed to register view: %v", err)
		return err
	}

	hc.exporter = newECSHealthCheckExporter()
	view.RegisterExporter(hc.exporter)
	return nil

so originally we used our view to register our new exporter, how can we do that in the new idea?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't use your view, you use the view package to register your exporter. See the fact that on line 44 you don't keep a reference on the view instance. You need to rely on the name in the exporter to access the data for this specific view.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, i will push 2 new pr for the changes in opentelemetry-collector and opentelemetry-collector-contrib repo, thanks!

@bogdandrutu bogdandrutu marked this pull request as draft July 14, 2021 15:49
# Conflicts:
#	service/defaultcomponents/defaults.go
@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants